Add Reentrancy-Guard modifier to external MP methods

[nicholaspai]

Fixes #1332
Fixes #1311

This leaves us with the following bytecode statistics:

Contract is 24235 bytes in size.
This leaves a total of 341 bytes within the EIP170 limit.

[nicholaspai]

Added because of call to registry.registerContract

[chrismaree]

So just to double-check: reentrancy would occur here if the party registered was a smart contract with a hook it could re-enter at this point? what would this enable it to do within the context of the _registerContract function?

[nicholaspai]

This is probably overkill, I was just putting nonReentrant on functions that made external calls. The reetrancy would be if the registered Registry address in the Finder was a smart contract that attempted to re-enter.

I can't think of an actual exploit they would do, but I suppose re-entrancy is possible.

        Registry registry = Registry(finder.getImplementationAddress(OracleInterfaces.Registry));
        registry.registerContract(parties, contractToRegister);

[nicholaspai]

I will remove though because its not an external method

[nicholaspai]

After discussions with the team, we concluded that in a perfect world we would add reentrancy-guards to all external methods, including view-only methods with the nonReetrantView() modifier. However, I'm running into practical limits with our contract deployed bytecode.

Currently, there are external methods in AddressWhitelist, SyntheticToken, PricelessPositionManager, and ExpiringMultiPartyCreator (and potentially elsewhere) that are no yet guarded by these modifiers. This is because doing so soon pushes the contract over the bytecode size.

As of the time of posting, ExpiringMultiPartyCreator is ~400 bytes short of the limit.

[nicholaspai]

I've added nonReentrant() to all state-modifying public methods in the EMP (NOT including any contracts in the common/ folder such as AddressWhitelist.sol.

This leaves the EMPCreator with < 100 bytes of bytecode remaining.

I'd still like to find a way to add nonReentrant() to the contracts in common/ that are exclusively used by the EMP, the constructors in the EMP, and adding nonReentrantView() to all public view-only methods. But the main obstacle right now is the bytecode limit.

[mrice32]

Would it help the bytecode size if we put all the logic in two (or just one) internal methods _preEntranceCheck() and _postEntranceReset() (the latter may not help/be necessary)? Obviously, it's messier, but it just wanted to try it in case it would slightly improve our bytecode issues (my worry is that the require call is taking a lot of bytecode and being copied around to every method).

[nicholaspai]

Wouldn't this have the same number of require statements per function call?

[mrice32]

The logic would be the same. But the way Solidity organizes the bytecode may be different. For instance, I think modifiers generally work this way:

function 1 start
modifier bytecode1 ( pre _; )
function 1 logic bytecode
modifier bytecode2 ( post _; )
function 1 end

function 2 start
modifier bytecode1 ( pre _; )
function 2 logic bytecode
modifier bytecode2 ( post _; )
function 2 end

...

The modifier bytecode would just be pasted into every function.
If you were to have the modifier call a function, it might look like this:

function 1 start
JUMP to modifier bytecode1
function 1 logic bytecode
JUMP to modifier bytecode2
function 1 end

function 1 start
JUMP to modifier bytecode1
function 1 logic bytecode
JUMP to modifier bytecode2
function 1 end

...

modifier bytecode1 (pre _; )
modifier bytecode2 (post _; )

In the latter, there's only one instruction to run each part of the modifier bytecode -- jumping to the function that the modifier calls. In the former, the entire modifier bytecode will be pasted for each function, bloating the bytecode. All the opcodes and data that make up that require statement may only be in one place in the bytecode rather than copied into every function. We've established in the past that require statements have a pretty big impact on the bytecode (especially when they have a message), so I thought eliminating the repetition may help -- although I'm not 100% sure of how solidity organizes this. It could require you changing the number of runs in the optimizer to see the impact.

[nicholaspai]

Ok this would help, but the overall bytecode impact would be small, likely < 1000 bytes. We'll run into the bytecode limit either way once we add back require messages. I think we should think bigger picture about how to organize the code to save bytecode.

I do think this change would help some and I'll test it out.

[nicholaspai]

This saved ~800 bytes, great idea @mrice32 !

[nicholaspai]

I've now applied the lock to all public methods including view-only methods. The one notable exception is pfc() which we cannot make nonReentrantView() because it is called by payRegularFees()

[mrice32]

I've now applied the lock to all public methods including view-only methods. The one notable exception is pfc() which we cannot make nonReentrantView() because it is called by payRegularFees()

Seems reasonable. You could technically create an internal and external version of pfc to mitigate this issue, but since it's not really used for anything outside of the contract (only used for informational purposes), I don't really see a reason to do so, especially since that would give us less bytecode space to work with :)

[nicholaspai]

I've now applied the lock to all public methods including view-only methods. The one notable exception is pfc() which we cannot make nonReentrantView() because it is called by payRegularFees()

Seems reasonable. You could technically create an internal and external version of pfc to mitigate this issue, but since it's not really used for anything outside of the contract (only used for informational purposes), I don't really see a reason to do so, especially since that would give us less bytecode space to work with :)

Yeah I thought about this too, it comes down to consistency versus bytecode. I'd opt for saving bytecode and I'll include a comment detailing why this public view function does not have t he modifier.

[mrice32]

I've now applied the lock to all public methods including view-only methods. The one notable exception is pfc() which we cannot make nonReentrantView() because it is called by payRegularFees()

Seems reasonable. You could technically create an internal and external version of pfc to mitigate this issue, but since it's not really used for anything outside of the contract (only used for informational purposes), I don't really see a reason to do so, especially since that would give us less bytecode space to work with :)

Yeah I thought about this too, it comes down to consistency versus bytecode. I'd opt for saving bytecode and I'll include a comment detailing why this public view function does not have t he modifier.

SGTM.

[mrice32]

Looks great! A few minor comments.

[mrice32]

nit: shouldn't this be _preEntranceSet()? Since it's before you enter the function? Or am I misunderstanding the naming convention?

[nicholaspai]

yep typo on my part

[mrice32]

1. Shouldn't the guard be applied at the outer-most layer rather than the inner most? Otherwise, can't you have re-entrancy once you leave the inner-most call?
2. Does this fail if you add this here? Isn't the Liquidatable constructor called before the re-entrancy guard goes into effect?

[nicholaspai]

1. I think your counterexample is correct. I applied it on the innermost because I didn't want someone to construct a PricelessPositionManager.sol or Liquidatable.sol whose constructor wasn't guarded by nonReentrant(). In practice this might not be super useful since we are forcing everyone to deploy EMP.sol's.
2. It doesn't actually fail if I add to FeePayer AND EMP, even though the bytecode does change (I verified). I ended up adding to just the top-level one in ExpiringMultiParty.sol.

[mrice32]

Same comment as above.

[mrice32]

Same comment as above.

[mrice32]

Now that we have the bytecode issue solved, should we do this just for consistency? Not super opinionated either way, but just wanted to mention it.

[nicholaspai]

Yeah thanks for the reminder. I added the modifier back and it ended up adding 25 bytes to the deployedBytecode FYI.

[mrice32]

Looks awesome -- one minor comment.

[mrice32]

Does nonReentrant() go into effect before or after the Liquidatable constructor gets called? Is it possible to put this modifier before the Liquidatable constructor is called? Or if it's not, maybe it makes sense to add nonReentrant() to all the constructors (since they shouldn't interfere with one another)?

[nicholaspai]

The constructors do not appear to interfere with each other so I just went ahead and put it on all of them.

[mrice32]

One minor nit.

[mrice32]

nit: why not just implement this in FeePayer? Then all the derived contracts would have to implement is _pfc()? Just feels a little weird implementing this here since there's nothing barring the base class from just implementing it once.

[nicholaspai]

done good point

[mrice32]

LGTM!