Call adapters using delegatecall to save gas

[mrice32]

This PR includes the updates in #51, so feel free to ignore the test changes.

This PR reworks how we interact with the adapters. It has significant flexibility tradeoffs as well as some security tradeoffs.

The main flexibility issues are that all variables in the adapter contracts must be immutable. This means that if there is a desire to change them, a new adapter would need to be deployed. However, since cross-chain contracts would point at the HubPool directly instead of the adapters (since the adapters don't actually call into the bridges from their own addresses), redeploying adapters wouldn't cause issues with cross chain communication.

The security tradeoffs are significant due to the Ethereum_Adapter contract. This contract essentially allows direct execution calls to be sent through it. These executions aren't relayed, they are executed directly from it, so running this inside a delegatecall means that the governance system can make any call from the HubPool that it desires. In the worst case, that would be transferring all tokens out.

The results of the gas test without delegatecall (just swapping the commented portions in this PR):

  Gas Analytics: HubPool Relayer Refund Execution
    Tree with 10 Leaves, each containing refunds for 10 different tokens
proposeRootBundle-gasUsed: 110232
      ✓ Simple proposal
executeRootBundle-gasUsed: 928862
      ✓ Executing 1 leaf (203ms)
(average) executeRootBundle-gasUsed: 899454
      ✓ Executing all leaves (1710ms)
(average) executeRootBundle-gasUsed: 712173
      ✓ Executing all leaves using multicall (1485ms)

The results with delegatecall:

  Gas Analytics: HubPool Relayer Refund Execution
    Tree with 10 Leaves, each containing refunds for 10 different tokens
proposeRootBundle-gasUsed: 110244
      ✓ Simple proposal
executeRootBundle-gasUsed: 719152
      ✓ Executing 1 leaf (172ms)
(average) executeRootBundle-gasUsed: 689744
      ✓ Executing all leaves (1392ms)
(average) executeRootBundle-gasUsed: 502463
      ✓ Executing all leaves using multicall (1238ms)

As seen here, the savings are significant.

[nicholaspai]

The main flexibility issues are that all variables in the adapter contracts must be immutable. This means that if there is a desire to change them, a new adapter would need to be deployed. However, since cross-chain contracts would point at the HubPool directly instead of the adapters (since the adapters don't actually call into the bridges from their own addresses), redeploying adapters wouldn't cause issues with cross chain communication.

I think redeploying adapters is a very reasonable way to upgrade Adapter state variables and is a better pattern.

The security tradeoffs are significant due to the Ethereum_Adapter contract. This contract essentially allows direct execution calls to be sent through it. These executions aren't relayed, they are executed directly from it, so running this inside a delegatecall means that the governance system can make any call from the HubPool that it desires. In the worst case, that would be transferring all tokens out.

This is clearly the big risk we are taking. Is there not a way to constrain which calls be executed via delegatecall? Do reentrancy guards not protect against this? I actually really like this pattern and would make upgrading really clean, so if we can add some protections here then I think LGTM

[nicholaspai]

so ugly, but neccessary. Alternatively, why not make the functions that need to pay ETH for arbitrum messages payable? I'm guessing its only the functions that call adapter.relayMessage?

[mrice32]

As discussed IRL, we can decide if we want to address this in a follow-up, but not a super high priority, as doing it this way may end up being more complicated.

[nicholaspai]

LGTM. One comment I have remaining on the loadEth method. Additionally, can you double check the deploy/ scripts to see if anything needs to change?

[mrice32]

LGTM. One comment I have remaining on the loadEth method. Additionally, can you double check the deploy/ scripts to see if anything needs to change?

I think it'd be better to do the deploy scripts in a follow up just to get this in more quickly.

[chrismaree]