Executor Improvements

[LayneHaber]

Summary

Remove the ReentrancyGuard for gas savings, no longer necessary with the onlyTransactionManager modifier. Update the order of actions. Adopt the contract calculation of relayerFee outlined here.

Motivation

There are a few improvements to the FulfillInterpreter that were recommended during the last audit:

1. Order of actions in failure case.
2. Reentrancy Guard.

Both are minor issues, and have gotten different feedback from different auditors, but should be addressed regardless.

Additionally, calculating the appropriate relayerFees necessary for executing arbitrary calldata is complex. The routers only care if they are relaying the transaction, but relayer networks will start to enforce these are sensible as well.

Proposed Solution

Failure Case

• Decrease the allowance before transferring the funds in the case where external calls fail

Reentrancy

• Remove the ReentrancyGuard and rely solely on the onlyTransactionManager modifier

Gas Costs

• The user needs to know when they prepare the transaction what the approximate cost of the execution of calldata on the receiving chain will be. This is because they must include enough in the SenderPrepare to cover the router fees, relayer fees, and be sufficient to get across chains
• Because the calldata is encrypted, nobody else can assess this ahead of time. Additionally, it may fail on .estimateGas calls until the Fulfill is done.
• Ultimately, this is within the purview of the user/contract executing the SenderPrepare. We should allow the user to pass in the gasPrice and the gasLimit for a given fulfill transaction to the sdk functions, and continue to emit the relayerFee used in events.

Test Cases

Not sure what test cases could be included to assess these changes outside of the expected scope of other tests.

Outstanding Questions

Tasks

[liu-zhipeng]

I think that is a good idea to remove ReentrancyGuard . which was not necessary with the onlyTransactionManager at all.

[LayneHaber]

Agreed! It was an oversight.

[LayneHaber]

Updated to reflect that #748 will not calculate costs onchain.

[LayneHaber]

Implemented in #865