omni / tokenbridge-contracts

Smart contracts for TokenBridge
http://docs.tokenbridge.net
GNU General Public License v3.0
230 stars 227 forks source link

Bypass the gas limit specified in arbitrary bridge message #586

Closed akolotov closed 3 years ago

akolotov commented 3 years ago

As soon as the safeExecuteSignatures functionality is implemented consider to extend the method call with an extra flag (or probably another method like safeExecuteSignaturesWithoutGasLimit) which will allow to pass to the target contract more gas and discard the gas limit specified in the arbitrary bridge message. This new logic should allow users to estimate the gas limit for transaction more precisely so the gas fees they see in applications like OB UI and MetaMask will be lower (in most cases).

k1rill-fedoseev commented 3 years ago

I don't think that executing a message with gas limit lower than pass alongside with AMB message is safe. In such cases, it is possible to fail any message execution by specifying extremely low gas limit (e.g. 1000 gas). I believe that gasLimit in the AMB message should be treated is the lowest guaranteed gas limit for the message execution.

UPD: I just figured out, that if safeExecuteSignatures will revert transaction on message failure, then it won't be possible to fail the message execution with OOG error. So, seems that there are no problems with that.

k1rill-fedoseev commented 3 years ago

I see the following implementations of such functionality: 1) safeExecuteSignaturesWithCustomGasLimit(bytes,bytes,uint256 gasLimit) - uses fixed gas limit passed in the third argument, reverts on message failure. This function will not be really useful in the UI, but I believe that allowing usage of fixed user-defined gas limits makes sense in some scenarios. 2) safeExecuteSignaturesWithAutoGasLimit(bytes,bytes) - passes all available gas to the message execution, reverts on message failure. This option will be useful in the UI, since it can be used together with eth_estimateGas for automatic gas limit estimation.

What do you think, @akolotov? Should we include both methods?