Across V3 and Oval Incremental Audit

OpenZeppelin Security | July 16, 2024
Table of Contents

Summary

Type
DeFi
Timeline
From 2024-05-20
To 2024-06-07
Languages
Solidity
Total Issues
25 (24 resolved, 1 partially resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
4 (4 resolved)
Low Severity Issues
7 (7 resolved)
Notes & Additional Information
14 (13 resolved, 1 partially resolved)

Scope

We audited the across-protocol/contracts repository at commit 95c4f92.

In scope were the following files:

 contracts
├── Blast_SpokePool.sol
├── chain-adapters
│   ├── Blast_Adapter.sol
├── erc7683
│   ├── ERC7683.sol
│   ├── ERC7683Across.sol
│   └── ERC7683Depositor.sol
├── handlers
│   ├── MutlicallHandler.sol
└── upgradeable
    └── MultiCallerUpgradeable.sol

In addition, the following PRs have been audited:

We also audited the UMAProtocol/oval-contracts repository at commit 297ab25.

In scope were the following files:

 src
├── adapters/source-adapters
│   ├── CoinbaseSourceAdapter.sol
├── controllers
│   ├── MutableUnlockersController.sol
├── factories
│   ├── BaseFactory.sol
│   ├── PermissionProxy.sol
│   ├── StandardChainlinkFactory.sol
│   ├── StandardCronicleFactory.sol
│   ├── StandardCoinbaseFactory.sol
│   ├── StandardPythFactory.sol
└── oracles
    └── CoinbaseOracle.sol

In addition, the following pull requests have been audited

System Overview

Across

Across is a cross-chain bridge acceleration protocol designed to facilitate the rapid transfer of tokens between Ethereum Layer 1 (L1) and the supported Layer 2 (L2) chains, as well as between L2 chains. Users of the protocol deposit funds to be transferred to their desired destination chain. Subsequently, third-party entities known as relayers fulfill the bridge orders by transferring their own funds to the specified destination address. Depositors receive the bridged amount, minus a system fee, while relayers are reimbursed by the protocol and additionally compensated with a reward for their services.

A SpokePool contract is deployed on each supported chain, with the HubPool contract on the L1 chain managing the overall liquidity within the system. The HubPool contract either transfers funds to the SpokePool contracts to facilitate relayer refunds or requests the return of excess funds from the SpokePool contracts back to HubPool. For a more detailed description of the protocol, you may refer to one of our previous reports.

Summary of Changes

During this audit, some new contracts added to the codebase are considered. These additions mainly concern:

  • Blast L2 integration. Blast is an OP-stack-based chain providing native yield for Ethereum and certain stablecoins. The Blast_Adapter contract facilitates the bridging of funds from L1 to Blast, while the Blast_SpokePool contract enables the bridging of funds from Blast to Ethereum L1 or other L2 chains supported by the protocol. Both contracts have been audited in this assignment.
  • Compliance with EIP-7683. EIP-7683 establishes a standardized API for cross-chain trade execution systems. It provides a generic CrossChainOrder struct, as well as a standard ISettlementContract smart contract interface. The Across protocol implements this standard while also being in full compliance with EIP-712.
  • The MutlicallHandler contract allows a user to specify a series of arbitrary calls that should be made upon receiving bridged funds via the message field in the Across deposit transaction.
  • Function depositExclusive in the SpokePool contract, which allows depositors to specify a fill deadline offset instead of a fill deadline timestamp. The offset value is added to the deposit transaction's timestamp to produce the final fill deadline.

Oval

Oval is a system designed to be used by the protocols which create MEV opportunities as a result of oracle price updates. Such opportunities are referred to as Oracle Extractable Value (OEV) and may arise in case of a price change that can cause liquidation in a DeFi protocol.

Oval introduces a mechanism which allows such protocols to capture some part of the OEV they generate. It involves providing intermediary smart contracts which may be plugged in between a DeFi protocol and the oracle being used. Such an intermediary contract fetches the data from an oracle and provides it to the target protocol. The price data to be provided may only be made available by a certain set of unlockers. A transaction which makes a price available to the target DeFi protocol may be back-run in order to extract OEV, for example, by liquidating a loan which became unhealthy after the price update.

These "price unlock" transactions are not submitted directly to the mempool. Instead, some information about them is shared in Oval nodes which run an auction for the right to be the first to use the price update. The transaction of the user who won the auction is then appended directly to the unlock transaction and passed to the Flashbots MEV-Share Node which, upon a successful execution of the received transaction bundle, refunds some percentage of funds paid for executing the bundle. This refund amount is split between UMA and the protocol integrating with it. More information about Oval may be found in one of our previous reports.

Summary of Changes

This audit focused on the code newly added to the protocol rather than on the entire codebase. The changes and additions which have been audited include:

  • Addition of the Coinbase API oracle and the Coinbase adapter allowing the Coinbase oracle to be used in Oval
  • Support for the Chainlink roundId parameter in the exposed data from the oracles
  • Addition of the factories deploying standard Oval instances for Chainlink, Chronicle, Coinbase, and Pyth
  • Addition of the PermissionProxy contract which, when set as a price unlocker, allows multiple whitelisted users to act as unlockers
  • Addition of the MutableUnlockersController contract, allowing the set of unlockers to be changed after the initial set-up
  • Introduction of the maxAge parameter limiting the staleness of the price data reported by the adapters

Security Model and Trust Assumptions

Across

The protocol uses the canonical bridges of the supported L2 chains in order to refund the relayers and relies on their infrastructure and functionality. By integrating the Blast L2 chain, its canonical bridge is also considered to work as expected. Across implements EIP-7683 and is expected to be used by external cross-chain execution systems. It is assumed that these external systems work as expected and behave in the best interest of their users. No further trust assumptions are introduced to the protocol with the new contracts audited in this assignment.

Oval

Oval relies on both on-chain and off-chain trust assumptions. The off-chain trust assumptions include:

  • Oval nodes process bundles and set refund instructions correctly.
  • Flashbots MEV-Share system does not un-bundle or leak MEV from back-runs.
  • Block builders respect MEV-Share's building rules.

In order for the Oval system to function correctly from the on-chain perspective, it is important that the unlockers frequently send the "unlock" transactions making the newest prices available. Moreover, it is trusted that the unlockers behave in the best interest of the protocol and its users, which includes not leaking the "unlock" transactions outside the Oval nodes and not utilizing them in order to extract OEV for themselves.

Furthermore, the newest update introduced the CoinbaseOracle contract which is responsible for fetching the price data from the Coinbase oracle to the protocols integrating with Oval. Since the Coinbase oracle is not deployed on-chain, it is essential that the prices provided by it are delivered to the CoinbaseOracle on a regular basis. Otherwise, stale prices may be returned.

Privileged Roles

Across

No further privileged roles have been introduced with the newly added contracts audited in this assignment.

Oval

In the Oval system, there are several privileged roles:

  • Updaters: Entities capable of unlocking the latest prices reported by the oracles.
  • Owner of the BaseController and MutableUnlockersController contracts: An account entitled to modifying the unlockers set and changing important parameters such as lockWindow, maxTraversal, and maxAge.
  • Owner of the PermissionedProxy contract: In case the PermissionedProxy contract is set as an unlocker, the owner may grant unlocking rights to additional accounts.
  • Owner of the Factory Contracts: Apart from the privileges of the owner of the controller contracts, the owner may additionally specify the set of default unlockers for each deployed controller instance.

It is trusted that each of these entities will behave in the best interests of the protocol and its users.

 

Medium Severity

[Across] It Will Not Be Possible to Bridge DAI to Blast

The relayTokens function of the Blast_Adapter contract is responsible for bridging tokens from Ethereum to Blast. In order to do that, it uses either the L1 standard bridge or the L1 Blast bridge, depending on the token being bridged. In case of DAI, an attempt to call depositERC20To function of the L1 Blast bridge is made. However, the bridge does not contain this function. This means that all transactions trying to bridge DAI to Blast using the relayTokens function will revert.

When DAI is being bridged, consider calling the bridgeERC20To function of the L1 Blast bridge instead.

Update: Resolved in pull request #518 at commit 9b9b3d6.

[Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail

The CoinbaseOracle contract serves as an oracle for price data reported and signed by Coinbase. In order to supply such price data to the contract, users have to call the pushPrice function which, in case of the correct data passed, updates the price data for a given asset. In order to verify the correctness of provided data, several checks are made, including the check for a valid kind field. However, this check reverts when kind is different than "price". The data signed by the Coinbase oracle will always contain kind equal to "prices", resulting in the pushPrice function always reverting, making it impossible to provide asset prices.

Consider verifying that the provided kind value is equal to "prices".

Update: Resolved in pull request #18 at commit ff31b1b.

[Oval] Newest Prices May Be Reported Ignoring the lockWindow() Constraint

The _tryLatestRoundDataAt function is invoked after the tryLatestDataAt function is called inside the internalLatestData function of the Oval contract. Its timestamp argument is equal to the maximum of the last unlock time and block.timestamp - lockWindow(). Before the introduction of the maxAge() parameter, it used to be that in cases where the price data had been unlocked inside the lockWindow() period, the price data would be returned. In the opposite case, the most recent price as of block.timestamp - lockWindow() timestamp would be returned.

Now, the maxAge() parameter has been introduced to limit the staleness of the data returned by the adapters. If the data to be returned by the _tryLatestRoundDataAt function is older than the maxAge(), the most recent price data is returned instead. This means that if the maxAge() is lower than the lockWindow() and the last price is unlocked within the lockWindow(), but its timestamp is older than the maxAge() parameter, the newest price will be returned by the internalLatestData function of the Oval contract.

However, this is not the desired behavior as the lock window mechanism has been introduced in order to guarantee that the "unlock" transactions unlock the most recent price that has not yet been used by anyone. As a consequence, the OEV opportunity for Oval will be lost since the newest price will be available for use by anyone before the "unlock" transaction takes place. A similar scenario may happen if the maxTraversal parameter is set to 0. In such a case, uninitialized data is returned by the _searchRoundDataAt functions inside the adapters and hence the check against the maxAge() value will never pass. As a result, the newest price data will always be returned.

Consider carefully validating the parameters with which the Oval contracts are deployed, especially ensuring that the max age is always bigger than the lock window and that the maxTraversal parameter is set to a reasonable value.

Update: Resolved in pull request #17 at commit 2aa2c7d.

[Oval] Updating Parameters Inside the BaseController Contract May Lead to Undesired Outcomes

The maxAge() parameter has been introduced to controllers in order to limit the staleness of the data returned by the adapters. Stale data could possibly be returned if no "unlock" transaction happens during the lockWindow() period. In such a case, the latest available price data for block.timestamp - lockWindow() is returned. The maxAge() parameter specifies the maximum age of such price data and, in case when the price is too old, the most recent price data is returned. Inside both ImmutableController and MutableUnlockersController, the maxAge() parameter is immutable and cannot be changed, but it is possible to change it inside the BaseController contract.

However, changing this parameter may lead to undesired results. One of the problems which may happen after such a change arises when the price data from an oracle is not unlocked for the lockWindow() time and the timestamp of the most recent price older than the lockWindow() is older than what the maxAge() parameter allows. In such a case, the newest price data is returned in order to avoid returning too stale prices. However, if maxAge() is then changed to a higher value, such that the most recent price as of block.timestamp - lockWindow() is not older than the maxAge(), that older price will be returned. It means that in the same block, the oracle adapter may return the most recent data and then the stale data, which is an unexpected and incorrect behavior.

The opposite scenario is also possible when the maxAge() parameter is decreased. In such a case, the most recent data will be unexpectedly returned which will take away the possibility of extracting OEV for the newest price for Oval by unlocking the price. It should be noted that these are just two of many different scenarios that may happen if the maxAge() parameter is changed. Similar problems may also arise when the unlockWindow() and maxTraversal() parameters are modified, even if maxAge() stays the same.

Consider setting the maxAge(), unlockWindow(), and maxTraversal() parameters as immutable inside the BaseController in order to avoid returning multiple different prices for the same timestamp.

Update: Resolved in pull request #19, at commit 0e94235. It is ensured that the reported price does not change when any of the lockWindow, maxAge and maxTraversal parameters is changed and also that maxAge is always a value greater than lockWindow.

Low Severity

[Across] Error-Prone Initialization of Blast_Adapter.sol

The constructor of the Blast_Adapter contract initializes the inherited CircleCCTPAdapter contract by providing the Circle domain ID of the Base chain. However, Blast is not yet supported by the CCTP protocol. The CircleCCTPAdapter library contract is currently inherited by the Blast_Adapter contract only in case of future use (when Blast integrates with CTTP). Thus, CircleCCTPAdapter should be dummy initialized at the moment.

Consider clearly initializing all CircleCCTPAdapter parameters with dummy values in Blast_Adapter to avoid possible erroneous configuration in a future version of the contract.

Update: Resolved in pull request #525.

[Across] Permit2 Witness Is Not Fully Compliant With EIP-712

The ERC7683Permit2Lib library contains EIP-712 typehashes of various types and helper functions for hashing data. However, the hashOrder and hashOrderData functions, which are used to calculate the Permit2 witness, do not comply with the EIP-712 standard as they use abi.encodePacked for encoding data which will not pad each member value to 32 bytes as required by the EIP-712 standard. As such, abi.encode should be used instead.

Moreover, since the message member of the AcrossOrderData struct is of type bytes, its keccak256 hash should be used for the encoding. However, the message member is used directly instead.

Additionally, the data passed to the permitWitnessTransferFrom function as the witnessTypeString argument should contain the custom witness declaration followed by encodings of all nested structs and the TokenPermissions struct in the alphabetical order. The reason for that is that both the custom witness and the TokenPermissions structs are members of the resulting PermitWitnessTransferFrom struct, whose encoding is then used for verifying the signature. As a result, both the TokenPermissions struct and all nested structs of the witness become the referenced struct types and because of that, should be sorted by name in the encoding of the PermitWitnessTransferFrom struct. However, the data passed to the permitWitnessTransferFrom does not follow this order.

Consider updating the hashOrder and the hashOrderData functions so that they return data that is compliant with EIP-712. Moreover, consider changing the order of the struct encodings in the data passed to the permitWitnessTransferFrom function, so that they are sorted alphabetically.

Update: Resolved in pull request #519 at commit 0dbde21 and in pull request #506 at commit e2d406f.

[Across] Users May Lose Assets if They Specify an Empty Address as a Call Target

The attemptCalls function of the MulticallHandler contract enables users to specify and execute custom operations on a target blockchain right after tokens are bridged. It performs these operations by doing a series of low-level calls to the target addresses with the given data. If any of these calls fails, the entire chain of transactions is reverted in order to protect users from losing their assets.

In Solidity, whenever high-level calls are made to an address without any code, the execution reverts. However, when low-level calls are made to an empty address, they are considered successful. It means that if a user mistakenly specifies an address not containing any code as a target, their transaction will succeed, whereas it would have failed if high-level calls had been made. This may cause users to lose their assets.

In order to illustrate this, consider an example where a user bridges token A from Ethereum to Blast and wants to immediately swap it to token B and send the received amount of B to their own account. In addition, suppose that the DEX they want to use has different address on these two blockchains. If a user mistakenly specifies the DEX address from Ethereum, the call made to this address (with no code) on Blast will be treated as successful and the subsequent transaction will send zero amount of B tokens to the user's account. All bridged A tokens will still remain in the contract, available to be stolen by anyone.

Consider validating the code size of a target address for each call with non-empty calldata in the attemptCalls function and revert in case it equals 0.

Update: Resolved in pull request #531 at commit 53d21b6.

[Across] Blast_SpokePool May Fail on Attempt to Bridge Native Blast Tokens

The _bridgeTokensToHubPool function handles bridging tokens from the Blast spoke pool to the hub pool. For each token different from USDB and WETH, the withdrawTo function of the standard bridge is called. However, this function will only work for ERC-20 tokens that are not native to Blast, as otherwise, the standard bridge will attempt to call the l1Token function which may not be implemented for native tokens and the transaction will revert. Furthermore, in the case of tokens native to Blast, it is expected that a sufficient allowance is given beforehand to the bridge as it calls safeTransferFrom in order to lock the tokens. However, currently, the Blast_SpokePool has no mechanism to approve tokens to the bridge.

Consider approving the relevant amount of tokens to the bridge and calling the bridgeERC20To function of the standard bridge (as described here) when bridging native Blast ERC-20 tokens from the spoke pool. Alternatively, if tokens native to Blast are not expected to be bridged in the future, consider documenting it.

Update: Resolved in pull request #520 and in pull request #536. The UMA team stated:

We decided to add a mapping to the Ovm_SpokePool to support native L2 token bridging. This will allow OVM chains like Blast to specify the corresponding remote L1 tokens.

[Oval] PermissionProxy.execute Will Fail if Non-Zero value Is Provided

The PermissionProxy contract is intended to be used as a proxy for calls that unlock new prices reported as a part of the MEV-share auction flow. It allows multiple different accounts to unlock new prices by performing calls to the controllers on their behalf. Calls may only be performed by authorized accounts and may be executed by invoking the execute function. Apart from the call target and the call data, it is possible to also specify the call value. However, the PermissionProxy contract has no function allowing it to receive the native value. This means that it is not possible for users to execute any call involving non-zero native value as all such calls would revert.

Consider making the execute function payable or removing the value argument if the PermissionProxy contract is not expected to make any calls with non-zero native value.

Update: Resolved in pull request #20 at commit 77e5441.

[Oval] CoinbaseSourceAdapter May Return Old Prices

In order to limit the staleness of the data returned by source adapters, the maxAge function has been introduced. It defines the maximum age of prices that can be returned by the tryLatestDataAt function. However, the CoinbaseSourceAdapter does not use the maxAge function when it returns historical data as other adapters do. This means that it can possibly return very old prices.

Consider limiting the staleness of the data returned by the CoinbaseSourceAdapter by utilizing the maxAge function.

Update: Resolved in pull request #24 at commit 70e5b66.

[Oval] Missing Input Validation

The deployer of the CoinbaseOracle contract sets the immutable reporter variable during the contract's construction. The reporter is the sole entity authorized to submit new prices to the oracle via the pushPrice function. The access control mechanism in pushPrice involves recovering the signer of the provided message and verifying that it matches the reporter's address. However, the ecrecover precompile does not fail on invalid signatures. Instead, it returns the zero address. If the reporter address is erroneously configured to the zero address, it becomes trivial for any entity to push arbitrary prices to the oracle contract.

Consider implementing a validation check to ensure that the reporter address is not set to the zero address to prevent erroneous contract deployments.

Update: Resolved in pull request #18 at commit b057756.

Notes & Additional Information

[Across] Lack of Indexed Event Parameters

Within MulticallHandler.sol, several events do not have indexed parameters:

To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.

Update: Resolved in pull request #532 at commit e2b80bd.

[Across] Unnecessary Cast

Within the Blast_Adapter contract, the IL1ERC20Bridge(L1_BLAST_BRIDGE) cast is unnecessary.

To improve the overall clarity, intent, and readability of the codebase, consider removing the unnecessary cast.

Update: Resolved in pull request #530 at commit 983531d.

[Across] Variable Names Inconsistent With Interfaces

Throughout the codebase, there are several instances of variables having different names than those in the original interface defining them:

  • The mode variable from the IERC20Rebasing has a different name than the corresponding variable declared in the configure function of the USDB implementation contract deployed on the Blast blockchain.
  • The recipient variable from the IBlast interface has a different name than the corresponding variable declared in the Blast implementation contract deployed on the Blast blockchain.
  • The settlerContract variable used for the calculation of CROSS_CHAIN_ORDER_TYPE has a different name than its counterpart in the CrossChainOrder struct.

While these inconsistencies do not have any negative impact on the behavior of the code, fixing them would make the code easier to read. As such, consider changing variable names so that they are consistent with the original ones in order to improve the readability of the codebase.

Update: Resolved in pull request #526 at commit 974e2d6.

[Across] Code Duplication

The SpokePool contracts of the chains based on the Optimistic Virtual Machine (OVM-based chains) share a significant amount of code, primarily utilizing the bridge contracts of Optimism. This common logic is encapsulated within the Ovm_SpokePool contract, from which all OVM-based SpokePools inherit. The _bridgeTokensToHubPool function in the Ovm_SpokePool contract handles the process of transferring funds from the L2 chain back to the HubPool via bridging.

The Blast_SpokePool contract, which is also OVM-based, inherits from the Ovm_SpokePool. However, it overrides the _bridgeTokensToHubPool function, despite retaining the majority of the code from the Ovm_SpokePool contract. The special handling of the SNX token within the Ovm_SpokePool appears to necessitate the overriding of the entire _bridgeTokensToHubPool function, resulting in extensive code duplication.

Consider relocating the part of the code specifically responsible for handling the SNX token to the relevant OVM-based SpokePool contract. Doing this will enable the Blast_SpokePool contract to invoke the inherited _bridgeTokensToHubPool function and avoid code duplication.

Update: Resolved in pull request #520, pull request #524. The SNX specific logic has been relocated to the Optimism_Spokepool contract and code duplication has been mitigated.

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, there are contracts that do not have a security contact.

Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Resolved in pull request #535 at commit 8043c74.

[Oval] State Variable Visibility Not Explicitly Declared

Throughout the codebase, there are state variables that lack explicitly declared visibility:

For clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.

Update: Resolved in pull request #21 at commit 2552ef0 and in pull request #18 at commit b057756.

[Across] Orders May Be Blocked for Some Time

The ERC7683OrderDepositor contract allows initiating cross-chain orders by providing users' signatures for these orders to the initiate function. This function is callable by anyone and, apart from the order to be initiated and the user's signature, it also allows a caller to specify the filler data which is then used to fill the exclusive relayer data for the order. The exclusive relayer is an account which is exclusively allowed to fill a certain deposit before the exclusivity deadline passes.

However, in contrast to the specified order, the fillerData is not validated in any way inside the initiate function which means that it may contain an arbitrary value. This allows an attacker to specify a random address, making it impossible for the order to be filled until the exclusivity deadline passes.

Consider limiting the exclusivity deadline period so that this attack is not problematic for the users.

Update: Partially resolved in pull request #515. The UMA team stated:

This no longer becomes an issue once pull request #515 is merged because the depositor now includes in their CrossChainOrder.order bytes information about an exclusivityDeadlineOffset. The filler that then calls initiate() to bring the deposit on chain can fill in any account they wish as the exclusiveRelayer but the depositor can protect themselves from malicious relayers by setting a short deadline offset. Moreover, the channel through which the depositor and relayer communicate will presumably be designed to penalize fillers who misbehave and set any part of the fillerData with griefing intent

[Across] Multiple Contract Declarations Per File

Within ERC7683Depositor.sol, more than one contract, library, or interface have been declared.

Consider separating the contracts into their own files to make the codebase easier to understand for both developers and reviewers.

Update: Resolved in pull request #527 at commit 74b6c90.

[Across] Typographical Errors

Throughout the codebase, there are several instances of typographical errors:

In the ERC7683.sol file:

  • In line 12, "receive" should be changed to "received".

In the ERC7683Depositor.sol file:

  • In line 26, "quoteBeforeDeadline" should be replaced with "QUOTE_BEFORE_DEADLINE".

In the Blast_Adapter.sol file:

  • In lines 64 and 65, "Base" should be changed to "Blast".
  • In line 89, "blast" should be replaced with "standard".

In the MulticallHandler.sol file:

  • In line 52, "drainRemainingTokens" should be changed to "drainLeftoverTokens".

In the Blast_SpokePool.sol file:

  • In line 9, the word "can" is written twice.

Consider fixing all instances of typographical errors in order to improve the readability of the codebase.

Update: Resolved in pull request #529 at commit bfb8596.

[Oval] Typographical Errors

Throughout the codebase, there are several instances of typographical errors:

In the PermissionProxy.sol file:

  • In line 8, the word "allows" should be removed.
  • In line 10, the word "oval" should be written starting with a capital letter.

In the MutableUnlockersController.sol file:

  • In line 8, "can be change" should be "can be changed".

Consider fixing all instances of typographical errors in order to improve the readability of the codebase.

Update: Resolved in pull request #23 at commit d977126.

[Across] Prefix Increment Operator ++i Can Save Gas in Loops

Throughout the codebase, there are multiple opportunities where the subject optimization can be applied:

  • The i++ in MultiCallerUpgradeable.sol
  • The i++ in MultiCallerUpgradeable.sol
  • The i++ in MulticallHandler.sol

Consider using the prefix increment operator (++i) instead of the postfix increment operator (i++) in order to save gas. This optimization skips storing the value before the incremental operation as the return value of the expression is ignored.

Update: Resolved in pull request #522 at commit d70155c and in pull request #534 at commit 47e59f7.

[Across] Misleading Comments

The following instances of misleading comments have been identified:

  • In Blast_SpokePool.sol, the comment inside the _claimYield function suggests that the native yield received from the yield contract is converted to WETH, but it is not, as ETH is transferred directly to the recipient.
  • In Blast_SpokePool.sol, in this comment, it might not be immediately clear what the word "this" refers to. 0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000 is the address of the LegacyERC20ETH contract. However, it might be understood from the comment that it is the address of the canonical bridge.

Consider correcting the comments in order to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #528 and pull request #520.

[Oval] Misleading Comment

The following misleading comment has been identified:

In ChainlinkSourceAdapter.sol, this comment says that if the data returned by the _searchRoundDataAt function is uninitialized, the current price data is returned. However, the latest price data will also be returned in case the _searchRoundDataAt function returns initialized data that is older than maxAge().

Consider fixing the comment in order to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #25, at commit 5624e60.

[Oval] Possible Duplicate Event Emission

When a setter function does not check if the value has changed, it opens up the possibility of spamming events indicating that the value has changed when it has not. Spamming the same values can potentially confuse off-chain clients.

Within MutableUnlockersController.sol, the setUnlocker function sets the unlockers state variable and emits an event without checking if the value has changed.

Consider adding a check to revert the transaction if the value remains unchanged.

Update: Resolved in pull request #26, at commit 6f53172.

Conclusion

Across is a protocol enabling rapid token transfers between Ethereum L1 and L2 chains. Users deposit funds, relayers transfer the funds to the destination, and depositors receive the amount minus a fee. The HubPool contract on L1 manages liquidity, coordinating with SpokePool contracts on each supported chain. On the other hand, Oval captures Oracle Extractable Value (OEV) generated by price updates in DeFi protocols. It uses intermediary contracts to fetch and provide price data, with transactions being auctioned to control the distribution of the extractable value. New additions in the Oval protocol include the Coinbase API oracle, support for Chainlink roundId, and contracts for managing unlockers and data staleness.

The audit yielded several medium- and low-severity issues. In addition, various recommendations have been made to avoid possible misconfigurations that could lead to faulty contract deployments and errors in integrating with external infrastructure.

Both Across and Oval teams have provided useful documentation, detailed explanations, and valuable insights during the audit process.

 

Request Audit

 

Related Posts