Prevent reentrancy by disallowing default context values #363
Conversation
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looking good, although I think we need to make one minor change in how we prevent this input, see comment below.
One option here is to have an explicit uint constant DEFAULT_BLOCKNUM_VAL here which is used both in this check and _resetContext? Seems like that would be a little more robust in future situations where we would want to change the default value, but OTOH there could be other complications and it's a bit ugly. Thoughts?
| @@ -159,7 +159,9 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |||
| override | |||
| public | |||
| { | |||
| require(transactionContext.ovmNUMBER == 0, "Only be callable at the start of a transaction"); | |||
| require(transactionContext.ovmNUMBER == 0, "Only callable at the start of a transaction"); | |||
| require(_transaction.blockNumber != 0, "Prevent reentrancy by disallowing default context values"); | |||
There was a problem hiding this comment.
Agree that this is a fine way to go, although we need to make sure this is handled the same way as an invalid gas limit in run. This is less safe because if you can ever satisfy this require's condition, then you could make proving fraud impossible, since reverts are reserved for INVALID_STATE_ACCESS. Should probably break this and the gaslimit one into a shared internal function.
Also use a default context value for ovmNumber and blockNumber
| @@ -937,7 +937,7 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad | |||
| internal | |||
| view | |||
| { | |||
| // If there are existing elements, this batch must have the same context | |||
| // If there are existing elements, this batch must have the same context | |||
There was a problem hiding this comment.
once again, editorConfig wuz here
| * Default Context Values * | ||
| **************************/ | ||
|
|
||
| uint256 constant DEFAULT_UINT256 = 0xdefa017defa017defa017defa017defa017defa017defa017defa017defa017d; |
There was a problem hiding this comment.
Previously in #244, I used MAX_UINT, but I feel uneasy about being 'off-by-one' from zero.
I also wanted a number that can be reused as the default for any of the elements in a TransactionContext or MessageContext.
I like this one, and I don't think it could ever be a valid context number.
| @@ -1801,7 +1836,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |||
| { | |||
| transactionContext.ovmL1TXORIGIN = address(0); | |||
| transactionContext.ovmTIMESTAMP = 0; | |||
| transactionContext.ovmNUMBER = 0; | |||
| transactionContext.ovmNUMBER = DEFAULT_UINT256; | |||
There was a problem hiding this comment.
It kind of expands the scope, but I can see an argument fo applying the change to the other values.
There was a problem hiding this comment.
It does seem a bit weird for this to be applied to ovmNUMBER and not to other values. If we do decide to keep the scope limited, we should at least add a comment explaining exactly why we do this for ovmNUMBER and not elsewhere.
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Outdated
Show resolved
Hide resolved
| @@ -1801,7 +1836,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |||
| { | |||
| transactionContext.ovmL1TXORIGIN = address(0); | |||
| transactionContext.ovmTIMESTAMP = 0; | |||
| transactionContext.ovmNUMBER = 0; | |||
| transactionContext.ovmNUMBER = DEFAULT_UINT256; | |||
There was a problem hiding this comment.
It does seem a bit weird for this to be applied to ovmNUMBER and not to other values. If we do decide to keep the scope limited, we should at least add a comment explaining exactly why we do this for ovmNUMBER and not elsewhere.
| // using the standard nonReentrant pattern. | ||
| if (_transaction.blockNumber == DEFAULT_UINT256) return false; | ||
|
|
||
| if (_isValidGasLimit(_transaction.gasLimit, _transaction.l1QueueOrigin) == false) return false; |
There was a problem hiding this comment.
Hmmm could we just move the logic from _isValidGasLimit into this function instead?
There was a problem hiding this comment.
FWIW I like keeping it separate; there is a good meaty chunk of logic in there especially with the gas rate limiting stuff.
| // Make sure that run() is not re-enterable. This condition should awlways be satisfied | ||
| // Once run has been called once, due to the behvaior of _isValidInput(). | ||
| if (transactionContext.ovmNUMBER != DEFAULT_UINT256) { | ||
| return; |
There was a problem hiding this comment.
Hmmm is it ok to return here? If so, why were we reverting before?
There was a problem hiding this comment.
I bellieve we explicitly should not have been reverting here, because it would've allowed a malicious caller to revert with malformed data.
Fixes: https://github.com/ethereum-optimism/roadmap/issues/803