refactor: [M04/L03/N16] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths #89

nicholaspai · 2022-03-14T22:00:42Z

Issue:

[M04] whitelistedRoutes for Ethereum_SpokePool affect other routes
- When in HubPool's executeRootBundle function, tokens are moved between spokes in order to
  complete rebalances of the different spoke pools. These token transfers happen within the
  _sendTokensToChainAndUpdatePooledTokenTrackers function, but in order to complete a rebalance
  the route from the chainId of the HubPool to the destination chain must be whitelisted.
  The issue comes from the conflation of two slightly different requirements. When whitelisting a route, a
  combination of origin chain, destination chain, and origin token are whitelisted. However, when
  rebalancing tokens, the specific route where origin chain is the HubPool's chain must be whitelisted for
  that token and destination chain pairing.
- This means that if other routes are to be enabled for rebalancing, the route from the
  Ethereum_SpokePool to some destination chain's SpokePool must be enabled as well. This may allow
  undesired transfers to the Ethereum_SpokePool. Additionally, it may cause problems if some token is
  to be allowed to move between chains aside from Ethereum, but specifically not Ethereum. It would be
  impossible to disable transfers to the Ethereum_SpokePool without also disabling transfers between
  separate spoke pools for the same token.
- Also note that whitelisting a route does not necessarily whitelist the route from Ethereum to the same
  destination chain. This means that a separate transaction may need to be sent to enable rebalances to/
  from that destination, by whitelisting the Ethereum-as-origin route. This is confusing and could lead to
  unexpected reversions if forgotten about.
- Consider modifying the whitelist scheme so that rebalances to specific chains are automatically
  enabled when enabling certain routes. For example, if the route for some token to move from Arbitrum
  to Optimism is enabled, then the route from the Hub to Optimism should also be enabled. Additionally,
  consider implementing some special logic to differentiate routes from the HubPool and routes from the
  Ethereum_SpokePool, so that either route can be enabled independently of the other.
[L03] No good way to disable routes in HubPool
- Within the SpokePool there exists the enabledDepositRoutes mapping, which lists routes that have
  been approved for deposits (allowing a user to deposit in one spoke pool and withdraw the deposit from
  another). The setEnableRoute function can be used to enable or disable these routes.
  Within the HubPool, there is a separate whitelistedRoutes mapping, which determines whether
  tokens can be sent to a certain spoke during rebalances. The only way to affect the
  whitelistedRoutes mapping is by calling whitelistRoute, which includes a call to enable the
  originToken/destinationChainId pair within the Spoke. This means that there is no good way to
  disable a whitelisted route in the hub without "enabling" the same route in the enabledDepositRoutes
  mapping in the SpokePool.
- Assuming that there may be cases in the future where it would be desirable to disable a certain deposit
  route, consider adding a function which can disable a whitelistedRoutes element (by setting the
  value in the mapping to address(0)) without enabling the route in the SpokePool. It may be desirable
  to disable both atomically from the HubPool, or to establish a procedure to disable them independently
  in a specific order. Consider designing a procedure for valid cross-chain token transfers in the case that
  only one mapping has a certain route marked as "disabled", and including this in the UMIP for dispute
  resolution. Finally, note that any "atomic" cancellations will still include a delay between when the
  message is initiated on the hub chain and when execution can be considered finalized on the spoke
  chain.
[N16] whitelistedRoute can be external
- The whitelistedRoute function within HubPool is marked as public. However, it is not called
  anywhere within the codebase.Consider restricting the function to external to reduce the surface for error and better reflect its intent.

Resolution:

This PR removes the HubPool's conflation of two different meanings of what a “whitelisted route” is. We now separate poolRebalanceRoutes, stored on the HubPool, from enabledDepositRoutes, stored on the SpokePool.
The PR means that the Ethereum_SpokePool deposit routes no longer are affected by pool rebalance routes stored on the HubPool.
This PR also makes it possible to disable both poolRebalanceRoutes (by zero-ing out destination tokens on the HubPool) and deposit routes (by flipping enabled bool to false).
Replaces whitelistedRoute with poolRebalanceRoute on the HubPool and makes it external.

contracts/HubPool.sol

mrice32

Looks great! Just a few minor comments and questions.

contracts/HubPool.sol

mrice32 · 2022-03-15T05:45:01Z

contracts/HubPool.sol

+    ) public override onlyOwner {
+        whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)] = Route({
+            destinationToken: destinationToken,
+            depositsEnabled: depositsEnabled


Is this just meant for offchain applications to read?

depositsEnabled is meant for off-chain agents only yes.

contracts/HubPool.sol

mrice32

Looks good!

mrice32 · 2022-03-15T16:15:11Z

contracts/HubPool.sol

+     * @param originToken Token sent from origin chain.
+     * @param depositsEnabled Set to True/False to enable/disable route on SpokePool for deposits.
+     */
+    function relayWhitelistRoute(


I'm not sure of a better way to do this, but it does seem a bit clunky and error-prone to have two separate methods for setting the mapping and sending that setting to the spoke since we would almost always want to do these together. No real suggestion here, just thinking that this might add frustration to the management of the protocol as the number of chains and tokens increases.

What if we added a third function to combine the two methods? The other way is to add flags as function params to fine tune the logic. I can see how we'd want to only whitelist on the HubPool/SpokePool in certain situations so I think this flexibility is important

Technically we could call both simultaneously with multicall, it just might be annoying, especially if this is called via a gnosis multisig or governance system.

I could see a flag param also making sense.

Curious what @chrismaree thinks.

The trade off is that the flag param adds gas, but condenses three functions into one. I probably lean towards usability and adding the following bool params to the whitelistRoute call: bool relayToSpokePool, bool setInHub

I had another idea. What if we didn't store the depositsEnabled boolean on HubPool and only stored it on SpokePools? This would force relayers to use SpokePool whitelisted routes as the source of truth for determining which deposit routes are enabled, while the HubPool whitelist would only store pool rebalance routes.

The advantage is that the separation between pool rebalance and deposit routes would be more cleanly defined.

I see. The hub pool would still need an address map, right?

I think that your proposal for adding a bool to whitelistRoute that, when set, also enables the whitelist route makes a lot of sence as I agree having these separate as two functions is a pain as we will almost always be calling both.

had another idea. What if we didn't store the depositsEnabled boolean on HubPool and only stored it on SpokePools? This would force relayers to use SpokePool whitelisted routes as the source of truth for determining which deposit routes are enabled, while the HubPool whitelist would only store pool rebalance routes.

I like this idea but I worry that it drastically increases the L1->L1 calls we need to do as well as conflating the information on the L2 side. This might not be as bad as I think though...There are some other nice benefits to this as well. When making a deposit the FundsDeposited will then also be able to contain the DestinationToken which right now it does not as this information is not there.

your proposal would look something like this, right?
change setEnableRoute to have destinationToken as well and then modify enabledDepositRoutes to be originToken->destinationChainId->destinationStruct where destinationStruct contains destinationToken and depositsEnabledenabled. Then, we can remove this bit of logic from the HubPool as the only place that needs to know if a route is enabled is the deposit side (the hub pool does not care!).

In fact in this pattern what is required to be left on the HubPool? almost nothing, no?

actually I was proposing to store originToken -> destinationChainId -> depositsEnabled on the SpokePool (i.e. keep it as is now) and then store hash(originChainId, originToken, destinationChainId) -> destinationToken on the HubPool since we need the destination token for pool rebalances.

But I see why you now recommend we also store destinationToken on the SpokePool. Thoughts?

@chrismaree , @mrice32 and I talked off-line and we decided to remove the destinationToken from the SpokePool as we think the information is redundant and could lead to a situation where we set up the pairwise mappings incorrectly between SpokePools. This could cause routes between SpokePools to be inconsistent. Instead, we can use the poolRebalanceRoute on the HubPool as the source of truth for L1 --> L2 token mappings.

mrice32

#89 (comment)

into npai/refactor-whitelisted-routes

…sit routes atomically

nicholaspai · 2022-03-16T15:23:12Z

contracts/HubPool.sol

+     * this value is false, the pool rebalance destination tokens will be set to 0x0 and the deposit routes will be
+     * disabled on the SpokePools.
+     */
+    function setDepositAndPoolRebalanceRoute(


setDepositRoute and setPoolRebalanceRoute both average ~50,000 gas, while this method averages ~100,000 gas, so calling this method instead of calling setDepositRoute 2x and setPoolRebalanceRoute 2x saves 50% gas!

chrismaree · 2022-03-16T15:49:23Z

contracts/HubPool.sol

+            )
+        );
+
+        emit SetPoolRebalanceRoute(depositRouteChainId_1, ethereumCounterpartToken, l2Token_1);


If you are going to whitelist/enable routes in both directions would it make sence to create some kind of condensed event rather than emitting multiple times?

I think clients should be able to reliably query SetEnableDepositRoute and SetPoolRebalanceRoute without caring how they were emitted (i.e. via a batch method or not)

I did end up putting these emits into internal functions so you won't see multiple emits in a single method anymore

into npai/refactor-whitelisted-routes

nicholaspai · 2022-03-16T21:15:56Z

contracts/SpokePool.sol

    event SetXDomainAdmin(address indexed newAdmin);
    event SetHubPool(address indexed newHubPool);
-    event EnabledDepositRoute(address indexed originToken, uint256 indexed destinationChainId, bool enabled);
+    event EnabledDepositRoute(uint256 indexed destinationChainId, address indexed originToken, bool enabled);


I think this packs variables better? or maybe it doesn't matter for events?

dont think it makes a big diff

chrismaree

LGTM. let's get this in.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32

LGTM!

nicholaspai added 2 commits March 14, 2022 16:55

feat: Refactor whitelistedRoutes struct

85b944d

Update HubPool.sol

98967d2

nicholaspai requested review from chrismaree and mrice32 March 14, 2022 22:00

Update HubPool.sol

c02b15e

nicholaspai commented Mar 14, 2022

View reviewed changes

contracts/HubPool.sol Outdated Show resolved Hide resolved

mrice32 approved these changes Mar 15, 2022

View reviewed changes

nicholaspai changed the title ~~refactor: whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths~~ refactor: [M04] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths Mar 15, 2022

nicholaspai changed the title ~~refactor: [M04] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths~~ refactor: [M04/L03/N16] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths Mar 15, 2022

nicholaspai added the OZ Audit - March Resolves issue discovered in March 2022 OZ Audit label Mar 15, 2022

nicholaspai requested a review from mrice32 March 15, 2022 11:49

mrice32 approved these changes Mar 15, 2022

View reviewed changes

nicholaspai requested a review from mrice32 March 15, 2022 17:01

mrice32 reviewed Mar 15, 2022

View reviewed changes

nicholaspai requested a review from mrice32 March 15, 2022 18:06

nicholaspai added 6 commits March 16, 2022 10:01

drop depositsEnabled from HubPool

3b4e0f6

Update SpokePool.sol

4175d66

split up whitelistRoute

2955f32

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

055155c

into npai/refactor-whitelisted-routes

Fix

7025fc3

Add convenience method for admin to whitelist pool rebalance and depo…

7aa0b2f

…sit routes atomically

nicholaspai commented Mar 16, 2022

View reviewed changes

chrismaree reviewed Mar 16, 2022

View reviewed changes

Add bi directional helper method alongside single direction helper

2b2c0ec

nicholaspai requested a review from chrismaree March 16, 2022 16:44

nicholaspai mentioned this pull request Mar 16, 2022

feat: add fill logic across-protocol/relayer#4

Merged

nicholaspai added 4 commits March 16, 2022 17:03

remove destinationToken from spokePool

0bbf208

Update HubPool.sol

d257d98

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

ef14afb

into npai/refactor-whitelisted-routes

Update Arbitrum_SpokePool.sol

2282c96

nicholaspai added 2 commits March 16, 2022 17:07

Update Arbitrum_SpokePool.sol

f7c95ff

Update Arbitrum_SpokePool.sol

9bdff27

nicholaspai commented Mar 16, 2022

View reviewed changes

nicholaspai added 2 commits March 16, 2022 17:17

fix

38978da

Update HubPool.ExecuteRootBundle.ts

f5a8e74

chrismaree approved these changes Mar 17, 2022

View reviewed changes

chrismaree added 3 commits March 17, 2022 11:38

release

8ed603c

Signed-off-by: chrismaree <christopher.maree@gmail.com>

comment nit

62d85bb

Signed-off-by: chrismaree <christopher.maree@gmail.com>

comment nit

fb3f7a4

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32 approved these changes Mar 17, 2022

View reviewed changes

Final fix

2d0adf7

nicholaspai merged commit d21239f into master Mar 17, 2022

nicholaspai deleted the npai/refactor-whitelisted-routes branch March 17, 2022 13:29

This was referenced Apr 6, 2022

feat: clean up some test related inconsistancies #130

Merged

feat: add rate model store on rinkeby #132

Merged

chrismaree mentioned this pull request Apr 20, 2022

chrismaree/add block number to export #143

Merged

This was referenced May 3, 2022

chrismaree/mainnet deployment #149

Merged

Latest kovan deployment #154

Merged

feat: add reverse look up for contract info from address #155

Merged

chrismaree mentioned this pull request May 10, 2022

chrisamree/mumbai deploy #157

Merged

This was referenced May 17, 2022

fix: address issue with weth on boba #159

Merged

fix: add haircut proposal #160

Merged

fix: address param ordering issue #163

Merged

refactor: [M04/L03/N16] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths #89

refactor: [M04/L03/N16] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths #89

Uh oh!

Conversation

nicholaspai commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismaree left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicholaspai commented Mar 14, 2022 •

edited

Loading