The UMA Across system provides a mechanism that, in effect, allows users to send funds between all supported chains without waiting for standard token bridge transfers to complete. We audited the UMA Across V2 Protocol over the course of 2 weeks, with 2 auditors, plus another auditor for 1 week.
The audited commit was bf03255cbd1db3045cd2fbf1580f24081f46b43a of the across-protocol/contracts-v2 repository.
The contracts in scope were (in the /contracts/ directory):
Arbitrum_SpokePool.solEthereum_SpokePool.solHubPool.solHubPoolInterface.solLockable.solLPTokenFactory.solMerkleLib.solOptimism_SpokePool.solPolygon_SpokePool.solPolygonTokenBridger.solSpokePool.solSpokePoolInterface.solchain-adapters/Arbitrum_Adapter.solchain-adapters/Ethereum_Adapter.solchain-adapters/Optimism_Adapter.solchain-adapters/Polygon_Adapter.solThe Across V2 system manages multiple contracts which hold funds and transfer them to each other. These are the HubPool and multiple SpokePools. The Spokes can exist on other chains, and thus there are standardized “adapters” for sending funds from the hub to the various spokes in order to have a predictable interface.
The system allows users to make deposits on one chain, specifying a desire to withdraw on a different chain and paying a fee. At any point, other users can “fill” this “relay”, supplying the original depositor with funds on a different chain and taking a small fee. The relayers are then refunded by the system. If relayers do not fill deposits, the system performs a “slow relay” in which funds are moved across cross-chain bridges to fill the deposit.
The system cannot easily pass messages across cross-chain bridges, so in order for the hub to understand the state of all spokes, and to transfer funds accordingly, merkle trees are produced representing the needed actions, such as rebalances and relayer refunds. These merkle trees are represented with their roots, where the full set of needed merkle roots is called the “root bundle”. These are optimistically validated – meaning that they are considered truthful if not disputed within a certain time window. Once the liveness period (in which other users can dispute a root bundle) passes, funds can be transferred between the hub and spokes by using merkle proofs to prove that the transfer was included in the root bundle.
The rules by which a root bundle is determined invalid are notably NOT a part of the smart contract system, and are instead decided by an outside system called the Optimistic Oracle. These dispute rules are to be codified into an UMIP (UMA Improvement Proposal) or multiple UMIPs. Therefore, much of the security of the system rests on the un-audited UMIP, and for the sake of the audit we treated the UMIP as a black box. During the audit, we provided the UMA team with suggestions and reminders for important security considerations when it comes to codifying the UMIP(s).
There is one admin for the whole system. This admin can make decisions regarding which chains have valid spokes, which tokens are enabled, and which tokens on some chain map to which tokens on some other chain. The admin also controls parameters such as the system fee percentage, where fees are directed, what the bond for proposing new root bundles is, how disputed root bundles are identified, and which tokens are allowed within the system. This role is intended to eventually be set to the UMA Governor contract (controlled by UMA token holders).
The Optimistic Oracle, which is controlled by UMA holders, has the ability to resolve disputes on root bundles. This means that if it is compromised, it is possible for disputes to not resolve correctly, and, more importantly, whoever can control the optimistic oracle can decide how funds are moved within the system. This is notably a feature of the greater UMA ecosystem, and incentives exist to keep the Optimistic Oracle honest.
As stated, many of the security properties of the system could not be evaluated as they are affected by UMIPs which are not contained in the scope of this audit. Much of the audit involved checking integrations with cross-chain bridges, and many of the findings in the audit arose from these. Many of the problems identified had to do with problems inherent to synchronising information across multiple chains. More serious issues arose from improper use of signature schemes and insufficient information being passed to distinguish information needed for a single chain when not on that chain.
Overall, we were impressed with the thoughtfulness and attention to edge cases that the UMA team apparently had when developing the protocol. We were also deeply appreciative of their responsiveness when it came to understanding the intent of certain parts of the protocol, and for elucidating the planned UMIP schema for validating root bundles. We appreciated their willingness to collaborate to find solutions and provide documentation to better explain the intent of the codebase.
The UMIP is an extremely crucial part of the system, and if designed poorly creates opportunities for loss of funds in the protocol. The UMIP will need to include robust dispute resolution mechanisms and encompass many different reasons for dispute. Once again, the UMIP was not audited as part of this engagement, though we did provide feedback where applicable to address security concerns that should be addressed by the UMIP.
Finally, there was an issue related to griefing which were identified as an unfortunate byproduct of the system design. The system intentionally does not “earmark” funds for any specific recipient, instead performing rebalances between spokes and allowing authorized users to pull funds from these spokes. Thus, there are potential issues in which a user would have to wait much longer than expected for their funds if the funds are routinely taken by other users before them. However, there is little advantage for an attacker to grief this way, as they pay a small fee to create a valid deposit each time they do. Additionally, this attack goes down in likelihood as liquidity for the specific token increases, as relays for tokens with high liquidity will typically be filled by relayers (instead of system funds) who can earn a profit by doing so. The result is that such greifing is really only a problem for extremely illiquid and centrally held tokens, which may simply not be allowed in the system.
In each root bundle, the slowRelayRoot represents all the slow relays in a batch, which could involve multiple tokens and spoke pools. A valid root bundle would ensure the poolRebalanceRoot has a leaf for every spoke chain. When this rebalance leaf is processed, the slowRelayRoot will also be sent to the corresponding spoke pool.
Notably, every spoke pool receives the same slowRelayRoot, which represents all slow relays in the batch across the whole system. When the slow relay is executed, the Spoke Pool does not filter on the destination chain id, which means that any slow relay can be executed on any spoke chain where the Spoke Pool has sufficient funds in the destinationToken. Consider including the destination chain ID in the slow relay details so the Spoke Pool can filter out relays that are intended for other chains.
Update: Fixed in pull request #79 as of commit 2a41086f0d61caf0be8c2f3d1cdaf96e4f67f718.
Depositors can update the relay fee associated with their transfer by signing a message describing this intention. The message is verified on the origin chain before emitting the event that notifies relayers, and verified again on the destination chain before the new fee can be used to fill the relay. If the depositor used a static ECDSA signature and both chains support the ecrecover opcode, both verifications should be identical. However, verification uses the OpenZeppelin Signature Checker library, which also supports EIP-1271 validation for smart contracts. If the smart contract validation behaves differently on the two chains, valid contract signatures may be rejected on the destination chain. A plausible example would be a multisignature wallet on the source chain that is not replicated on the destination chain.
Instead of validating the signature on the destination chain, consider including the RequestedSpeedUpDeposit event in the off-chain UMIP specification, so that relayers that comply with the event would be reimbursed. This mitigation would need a mechanism to handle relayers that incorrectly fill relays with excessively large relayer fees, which would prevent the recipient from receiving their full payment. Alternatively, consider removing support for EIP-1271 validation and relying entirely on ECDSA signatures.
Update: Fixed in pull request #79 as of commit 2a41086f0d61caf0be8c2f3d1cdaf96e4f67f718.
When a relayer fills a relay, they specify a repaymentChainId to indicate which chain they want to be refunded on. However, the repaymentChainId is not validated against any set of acceptable values. Instead, it is included in the _emitFillRelay event, which is used for generating root bundles in the system.
Since not all tokens may exist on all chains, and some chain ID’s may not exist or be a part of the Across V2 system, consider specifying valid values for repaymentChainId for a given token, and implementing logic similar to that for enabledDepositRoutes to use for checking repaymentChainId. Alternatively, consider specifying in the UMIP some procedures for root bundle proposers to determine whether a repaymentChainId is valid, and what to do if it is not. In this case, invalid repaymentChainIds may mean a repayment is simply not repaid – if this is chosen, ensure that this is made very clear in any documentation about the system, so that users are not surprised by losing funds.
Update: Acknowledged. The UMA team intends to address this off-chain. They state:
We believe that this issue can be resolved in a well-defined UMIP that lists valid repayment chain IDs (or points to where to find them), and provide a default repayment chain ID for invalid ones. For example, the UMIP could stipulate that any invalid repayment chain IDs are repaid on mainnet.
removeLiquidity behavior could lock fundsThe removeLiquidity function in the HubPool contract accepts a boolean argument sendEth. This should be set to true “if L1 token is WETH and user wants to receive ETH”.
However, if the “user” is a smart contract, even if the L1 token is WETH and the sendEth argument is true, WETH, not ETH, will ultimately be sent back.
This is the case because if sendEth is true, then the _unwrapWETHTo function is called. That function checks if the intended recipient is a smart contract, and, if so, sends WETH.
If the receiving smart contract has no mechanism to handle WETH and was only expecting ETH in return, as was explicitly specified by the sendEth argument submitted, then any WETH sent to such a contract could become inaccessible.
To avoid unnecessary confusion and the potential loss of funds, consider either reverting if a smart contract calls removeLiquidity with the sendEth argument set to true or modifying the _unwrapWETHTo function so that it can also be provided with and abide by an explicit sendEth argument.
Update: Fixed in pull request #90 as of commit a1d1269e8a65e2b08c95c261de3d074abc57444d and pull request #139 as of commit f4f87583a4af71607bacf7292fee1ffa8fc2c81d.
whitelistedRoutes for Ethereum_SpokePool affect other routesWhen 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.
Update: Fixed in pull request #89 as of commit 2d0adf78647070e4dd20690f67f46daaa6fc82c4.
chainId function is not virtualWithin SpokePool.sol, the function chainId is marked override. However, the comments above it indicate that the function should also be overridable, meaning that it should be marked virtual.
Consider marking the function virtual to allow overriding in contracts that inherit SpokePool.
Update: Fixed in pull request #82 as of commit cc48e5721ea444a22a84ddeeef8dcbfe191b112c.
Throughout the codebase there are functions lacking sufficient input validation. For instance:
HubPool contract the various admin functions will accept 0 values for inputs. This could result in the loss of funds and unexpected behaviors if null values are unintentionally provided.HubPool contract the setProtocolFeeCapture function does not use the noActiveRequests modifier. This could allow the protocol fee to be increased even for liquidity providers that have already provided liquidity.MerkleLib library the isClaimed1D function does not work as expected if an index is greater than 255. In such a case, it will return true despite the fact that those values are not actually claimed.SpokePool contract the deposit function does not enforce the requirement suggested by the deploymentTime comment which says that relays cannot have a quote time before deploymentTime.SpokePool contract the speedUpDeposit function does not restrict the newRelayerFeePct to be less than 50% like the regular deposit does. In practice, the _fillRelay function won’t accept a fee that is too high, but this should still be enforced within speedUpDeposit.PolygonTokenBridger contract the “normal” use case of send involves the caller, Polygon_SpokePool, evaluating if the token it is sending is wrapped matic in order to set the isMatic flag appropriately. However, for any other caller, if they forget to set this flag while sending wrapped matic, then their tokens would be unwrapped but not sent anywhere. For more predictable behavior, consider checking for wrapped matic inline rather than relying on the isMatic argument.To avoid errors and unexpected system behavior, consider implementing require statements to validate all user-controlled input, even that of admin accounts considering that some clients may default to sending null parameters if none are specified.
Update: Fixed with pull request #113 as of commit 4c4928866149dcec5bd6008c5ac8050f30898b7f and pull request #142 as of commit 2b5cbc520415f4a2b16903504a29a9992a63d41c.
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.
Update: Fixed in pull request #89 as of commit 2d0adf78647070e4dd20690f67f46daaa6fc82c4.
chainId requirementsThe PolygonTokenBridger contract’s primary functions are only intended to be called either on l1 or l2, but not both. In fact, calling the functions on the wrong chain could result in unexpected behavior and unnecessary confusion.
In the best case, the functions will simply revert if called from the wrong chain because they will attempt to interact with other contracts that do not exist on that chain. For example, calling the receive function (by sending the contract some native asset) could trigger reverts on Polygon, but not on Ethereum, because there is a WETH contract at the l1Weth address on the latter but not the former.
However, in the worst case, it is possible that such calls will not revert, but result in lost funds instead. For example, if a WETH-like contract was later deployed to the l1Weth address on Polygon, then the call would not revert. Instead, tokens would be sent to that contract and could remain stuck there.
Although the inline documentation details which function should be called on which chain, consider having the functions in this contract actively enforce these requirements via limiting execution to the correct block.chainid.
Update: Fixed in pull request #115 as of commit b80d7a5396d31662265bb28b61a1a3d09ed76760 and pull request #128 as of commit 811ac20674d28189fd01297c05ce5b9e89f7a183.
In the HubPool contract the enableL1TokenForLiquidityProvision function allows the contract owner to enable an l1token to be added to the protocol for liquidity pooling.
This is allowed even if the l1token is already currently enabled.
As this function also sets the lastLpFeeUpdate variable to the then-current block.timestamp, enabling an already enabled token will skip over the period of time since lastLpFeeUpdate was last set. As a result, any LP fees that should have been assessed for that time period would simply never be assessed.
Consider reverting if this function is called for an l1token that is already enabled.
Update: Fixed in pull request #94 as of commit b1a097748a82c3276619a06fa36358b574f843e1.
nonReentrantWe have not identified any security issues relating to reentrancy. However, out of an abundance of caution, consider marking the following public functions in the HubPool contract as nonReentrant. Consider that the nonReentrant modifier only works if both the original function, and the re-entered function are marked nonReentrant.
setProtocolFeeCapturesetBondsetLivenesssetIdentifierwhitelistRouteenableL1TokenForLiquidityProvisiondisableL1TokenForLiquidityProvisionaddLiquidityUpdate: Fixed. Partially addressed in pull request #62 as of commit a3b5b5600e53d2ae877a4c1c18d78aadb01ff2e6 and then fully addressed in pull request #92 as of commit 7aa2fa8f46f8d40512857f35dd3ac64587c61f18.
In the HubPool contract during a call to the disputeRootBundle function, if the bondAmount and finalFee values are the same, then the proposer bond passed to the optimistic oracle is zero.
When this happens, the optimistic oracle unilaterally sets the bond to the finalFee and then attempts to withdraw bond + final fee.
Since the HubPool only sets the allowance for the oracle to bondAmount rather than bondAmount + finalFee, this transfer will fail and, as a result, the proposal will be cancelled.
This means that in the situation where bondAmount and finalFee values are identical, every proposal will be cancelled. Consider documenting this situation, checking for it explicitly and reverting with an insightful error message. Additionally, consider trying to avoid the situation by reverting in the setBond function if the newBondAmount is equal to the finalFee or in the proposeRootBundle function if bondAmount is equal to the finalFee.
Update: Partially fixed in pull request #96 as of commit 671d416db0fe6d813e3761bda0e3132cb30a8e1d. The condition is checked in setBond but not in proposeRootBundle.
In the HubPool function _updateAccumulatedLpFees, the return value of getCurrentTime() is cast to a uint32 value. This means that the value will be truncated to fit within 32 bits, and at some point around Feb 6, 2106, it will “roll over” and the value returned by casting to uint32 will drop down to 0. This will set pooledToken.lastLpFeeUpdate to a much lower number than the previous lastLpFeeUpdate. Any subsequent time _getAccumulatedFees is called, the timeFromLastInteraction calculation will be exceedingly high, and all “undistributed” fees will be accounted for as accumulated.
Again, note that this issue will only occur starting in the year 2106. Consider changing the size of the cast from uint32 to a larger number, like uint64. This should be more than enough to not encounter limits within a reasonably distant future. Alternatively, consider documenting the behavior and defining a procedure for what to do if the system is still in operation when the uint32 limit is hit, or for shutting down the system before the year 2106.
Update: Fixed in pull request #95 as of commit 2f59388906346780e729f2b879b643941ea314c9.
Within the Ethereum_Adapter, there is a mention of copying code from “Governor.sol”. It appears that the contract in question is Governor.sol from the UMAprotocol/protocol repository.
Since it is a part of a separate repository, and it is possible that the code may change in the future, consider including a link to the file, including a commit hash, so that it can be easily referenced by developers and reviewers in the future.
Update: Fixed in pull request #97 as of commit ac9ed389914dc4249f488226fcd94d6d0b44aeb0.
struct definitionsThe PoolRebalanceLeaf struct is defined in HubPoolInterface.sol, while the RootBundle, PooledToken, and CrossChainContract structs are all defined in the implementation, HubPool.sol.
Consider defining all structs for HubPool within the same contract.
Update: Fixed in pull request #100 as of commit 9a98ce1ae5c8c5e95bcfa979666b980008d14d3f.
In the LpTokenFactory contract, the LP tokens it creates have inconsistent versioning in their metadata.
While the token symbol is prepended with Av2 (ostensibly for “Across version 2”), the token name is prepended only with “Across” and no version number.
Consider adding the version number to the token name, or, alteratively, leaving an inline comment explaining the decision to omit the version number.
Update: Fixed in pull request #101 as of commit 91a08a9bd2b47a1a1319aff8bda53349e8264ce3.
Although most of the codebase is thoroughly documented, there are a few instances where documentation is lacking. For instance:
HubPool contract the public unclaimedAccumulatedProtocolFees variable has no inline documentation.HubPoolInterface contract the inline documentation accompanying PoolRebalanceLeaf.netSendAmounts, although lengthy, could benefit from additional clarification around the case of negative values. It could clarify further that in such cases the actual netSendAmounts value is ignored, but it should match the amountToReturn parameter in the RelayerRefundLeaf.MerkleLib library are missing NatSpec @return statements.To further clarify intent and improve overall code readability, consider adding additional inline documentation where indicated above.
Update: Fixed in pull request #102 as of commit e2bfe128ff1a9aeed02bfcebe58a5880ad283698.
In the LpTokenFactory contract, when the createLpToken function is called, it creates a new ERC20 LP token and adds the msg.sender to the new token’s minter and burner roles. These role assignments use the magic values 1 and 2, which are the uint identifiers for the respective roles.
Rather than using these literal values to assign roles, consider using the the ExpandedERC20.addMinter and ExpandedERC20.addBurner functions.
Update: Fixed in pull request #103 as of commit e9d3419ac6eb609b0c9165cdeac3fbff58285d18.
HubPool lines 718-719 explain that the whitelistedRoute function returns whitelisted destination tokens, but does not mention that if the token is not whitelisted then the function returns address(0).PoolRebalanceLeaf struct appear to refer to a previous version of the struct, making them hard to follow. For example, line 17 implies there are two arrays above it (there is only one), and line 31 suggests there are multiple arrays below it (there is only one).HubPool.executeRootBundle states that the function deletes the published root bundle, however it does not.LPTokenFactory contract, the comments on lines 24 and 25 should say “msg.sender” or “the calling contract” rather than “this contract”.lpFeeRatePerSecond variable suggest that LP fees are released linearly. In fact, they are released sublinearly, because the _getAccumulatedFees function uses a fraction of the undistributedLpFees (which decreases over time for any given loan), rather than the total funds on loan.SpokePool above the definition of claimedBitmap state that there are 256x256 leaves per root. However, due to the indexing scheme in MerkleLib, there are a maximum of 2^248 different values of claimedWordIndex, with 256 different claimedBitIndexes. A more clear comment might explain that there are 256x(2^248) leaves per root.Consider correcting these comments to make the code easier to understand for reviewers and future developers.
Update: Fixed in pull request #109 as of commit 21cdccd5cbfffd4f120ab56c2691b8e961a8d323, pull request #104 as of commit 1148796377365a2de52fb89810f769ffb7f8c96f and pull request #138 as of commit c0b6d4841b86ba8acf3e4a3042a78a1307410e6a.
payable multicall function disallows msg.valueThe MultiCaller contract is inherited by the HubPool and SpokePool contracts. It provides the public multiCall function that facilitates calling multiple methods within the same contract with only a single call.
However, although it is designated as a payable function, it disallows any calls that send ETH, ie where msg.value is not zero.
This effectively makes the payable designation moot and the contradictory indications could lead to confusion.
In the context of the HubPool, specifically, relays destined for chains where ETH is required and where a call to loadEthForL2Calls is therefore necessary, will not be multi-callable.
Consider either explicitly noting this limitation, or removing both the require statement and the payable designation.
Update: Fixed in pull request #98 as of commit 7092b8af1da15306994ea760b9669a9bd1f776c1.
We have identified some areas of the code which could benefit from better naming:
HubPoolInterface.liquidityUtilizationPostRelay, the parameter token should be renamed to l1Token to better match other functions in the interface, as well as the function’s implementation in HubPool.RootBundle struct, requestExpirationTimestamp should be renamed to better indicate that it ends the “challenge period”. Consider renaming it to ChallengePeriodEndTimestamp or similar.RootBundleExecuted event in HubPool.sol only names one of its array parameters in the plural form, but when the event is emitted, all array parameters are named in the plural form. Consider changing the event definition so that all array parameters are pluralized.function whitelistedRoute is vague and does not indicate what it’s output will be. Consider renaming it to something like destinationTokenFromRoute to better match the return value.weth is used in Polygon_SpokePool.sol, it refers to wrapped MATIC. Consider renaming the weth variable in SpokePool.sol to wrapped_native_token to make it more generalizable. This will make Polygon_SpokePool less confusing and be more generalizeable for future SpokePools.executeSlowRelayRoot and executeRelayerRefundRoot functions are executing leaves and should be renamed accordingly.unclaimedPoolRebalanceLeafCount parameter of the ProposeRootBundle event should be renamed to poolRebalanceLeafCount, since it’s always the total number of leaves in the tree.RootBundleCanceled event names the last parameter as disputedAncillaryData, but the proposal is not necessarily disputed. It should just be ancillaryData._append function of the LpTokenFactory could be called _concatenate to better describe its functionality.onlyEnabledRoute modifier has a destinationId parameter that should be destinationChainId to match the rest of the code base.Consider following our renaming suggestions to make the codebase easier for developers and reviewers to understand.
Update: Fixed in pull request #105 as of commit 87b69cdf159a1db5ccfcaa9f27825dfa416e7158.
Although tokens must be enabled to be used in the system, it is important to define what may make a token troublesome so that which tokens can be whitelisted is easier to determine.
transferred will not match the amount received by the contracts in the system. Many spots in the code, such as in the addLiquidity function, assume the amount transferred in equals the amount received.SpokePool._executeRelayerRefundRoot, a failing transfer for one token could block all other refunds for the specified leaf.Consider documenting procedures for tokens which behave unexpectedly to be filtered for before whitelisting.
Update: Fixed in pull request #137 as of commit ba6e03974cf722d33b9fb2def4da578129f5baed.
immutableWithin the HubPool contract, the weth, finder, and lpTokenFactory variables are only ever assigned a value in the constructor.
Consider marking these values as immutable to better signal the fact that these values or not meant to change and to reduce the overall gas consumption of the contract.
Update: Fixed in pull request #108 as of commit cccb9556345edcc5d8fc3022ab64a5b368c8d810.
When the LpTokenFactory contract creates an ExpandedERC20 token contract, the factory becomes the owner of that token contract. The factory then proceeds to assign the minter and burner roles to the msg.sender. The factory remains the owner.
As this is a residual power that is no longer needed by the LpTokenFactory, consider reducing the number of addresses with privileged roles by transferring ownership to the msg.sender.
Update: Fixed in pull request #109 as of commit 21cdccd5cbfffd4f120ab56c2691b8e961a8d323.
In HubPool.sol:
In HubPoolInterface.sol:
In MerkleLib.sol:
In Polygon_SpokePool.sol:
In SpokePool.sol:
In SpokePoolInterface.sol:
Update: Fixed in pull request #110 as of commit 813cfeef126484e0ac5b7fb91225560c5edbff7c.
Throughout the codebase, when the safeTransferFrom function is used to transfer assets into the system from an external address there is an implicit requirement that the external address has already granted the appropriate approvals.
For instance:
proposeRootBundle function relies on safeTransferFrom which requires that HubPool has been granted an allowance of bondAmount bondTokens by the caller.addLiquidity function relies on safeTransferFrom, requiring that the HubPool has been granted an l1TokenAmount allowance of the caller’s l1Token.In favor of explicitness and to improve the overall clarity of the codebase, consider documenting all approval requirements in the relevant functions’ inline documentation.
Update: Fixed in pull request #111 as of commit 5a3ef77a22b81411a3616bb48acf063acabb4d2c.
Throughout the codebase, there are instances of unused code. For example:
proposerBondRepaid attribute of the HubPool contract’s RootBundle struct is never used. Consider removing it.Arbitrum_Adapter contract are never used. As the relevant state variables are immutable, consider setting all relevant values in the constructor and emitting these events then. Alternatively, consider adding comments indicating why events are declared but unused.L2GasLimitSet event in the Optimism_Adapter is never emitted. Consider emitting it in the constructor, removing it, or adding a comment indicating why it is declared but not used.HubPoolChanged event is never used.Update: Fixed in pull request #78 as of commit f7e8518050a12e478516da6622bcf2357bb2e802 and in pull request #99 as of commit d89b1fb8d491703ef63dae0b29d93abd29d501de.
The below list outlines contract import statements that are unnecessary:
WETH9 and Lockable imports are not used in the Ethereum_Adapter contract.CrossDomainEnabled, IL1StandardBridge, and Lockable imports are not used in the Polygon_Adapter contract.WETH9 and IERC20 imports are not used in the Arbitrum_Adapter contract.AdapterInterface interface is imported twice in the Arbitrum_Adapter contract.WETH9 and SpokePoolInterface imports are not used in the Ethereum_SpokePool contract.IERC20 import in the LpTokenFactoryInterface interface is unused.MerkleLib is imported twice in the SpokePool contract.Consider removing unnecessary import statements to simplify the codebase and increase overall readability.
Update: Fixed in pull request #112 as of commit d81295d3fd433a1f08fdd42c75a0aa3233a77dbe.
whitelistedRoute can be externalThe 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.
Update: Fixed in pull request #89 as of commit 2d0adf78647070e4dd20690f67f46daaa6fc82c4.
One critical issue was found. Some changes were proposed to follow best practices and reduce the potential attack surface. The contracts are highly dependent on a well-structured UMIP which determines the behavior of the Optimistic Oracle.
During the fix review process, the UMA team provided us with a list of additional pull requests for our review. We proceeded to review the following additional PRs related to the Across V2 codebase:
f7e8518050a12e478516da6622bcf2357bb2e802 added “Emergency admin features to pause proposals and executions of root bundles, and to delete root bundles from the spoke pool to prevent a single bad bundle from permanently breaking or disabling the system.”emergencyDeleteProposal function. We raised that this is counter to best practice and could potentially, lead to issues later. This was then addressed later in pull request #147 as of commit ee7714734aab4ed0457c813403a63e53c6438529.8cf240a147b7d0467418eb81b2d6e152d478d101 removes an extraneous fee. Specifically, it addresses the fact that the: “Slow relay charges 0 relayer fee % and refunds user this fee. The relayer fee is intended as a speed fee. The user shouldn’t pay this fee for capital that the relayer doesn’t loan them.”70c56813e908cb5d02c43501d7de6a2c01564dca made changes to prevent a Spoke Pool’s address from accidentally/intentionally being set as address(0).029406ec534da9979b63acf354e63394b4ce3a90 changed the sizes of various uints to better limit their range of values and to prevent them from holding values which are too high. This is related to issue L02.d2ca5b2f1f604e30083a20c72f40d971c4161c59 added a mapping to allow tokens on Optimism to be transferred across custom bridges rather than the standard bridge.data field to be populated was made, but ultimately decided against.248bb4d67dfb195b7077f8632f548fa3db808be5 added logic to prevent redundant relays of root bundles to spoke pools on L2.a09e56b554577da8b929d8043fc6cdfb654e2ecf made changes to fix reversions when transferring tokens to Arbitrum.811ac20674d28189fd01297c05ce5b9e89f7a183 made changes to fix token bridging transactions using Polygon’s two bridge implementations.ac18f6a3fc89bc861af183a0b731c89837cf84ba modified parameter indexing for events.a72519e0965fc298ada2d19942ec5806530988df implemented argument spreading rather than passing PoolRebalanceLeaf objects when executing a root bundle “to improve etherscan tx processing.”3ec3a7f990ee9a50a4a44f6baf893d38d2914b38 removed getRootBundleProposalAncillaryData functionality based on the fact that, even with the prior implementation of the function, off-chain information will still be required to dispute proposals.AncillaryData.sol is still being imported in HubPool.sol, though no longer used.5a31be8aac645085f59e20cbb17e2fb24ec24f85 removes the getRootBundleProposalAncillaryData function altogether since it was just returning a literal empty string.30ea0888b141c4085d7e30eab4beecd6c8fd9a62 bumped the Solidity compiler version to the latest.98237643f482d9333b394cbf3f2a2c075205b7ba made changes related to gas optimizations and storage packing.uint32 usages in for loops that increased gas consumption and unnecessarily increased the possibility for overflow. This concern was subsequently addressed in pull request #148 as of commit f6d5bc387d24da6fc1cd99de10700d744daf3f6a.709bf1d99e32e5a3bea7605c218020e9d6a1e1f5 suppressed solhint warnings (in as limited a manner as possible).