Ondo Finance - nirlin's results

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/SourceBridge.sol#L61-L82 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L85-L114

Vulnerability details

Impact

Users with account abstraction wallets have different address across different chains for same account, so if someone using account abstraction wallet bridge the asset, assets will be minted to wrong address and lost permanently

Proof of Concept

Account abstraction wallet have been on the rise for quite a time now and have alot of users, just look at the figures by safe wallet (one of the account abstraction wallet)

4.4 million users and 5.4 billion assets, there is very high risk that safe wallet users will try to bridge the assets and lost them.

Now look at the codebase and understand how the assets will be lost.

In source bridge in burnAndCallAxelar we construct the payload as follow :

here we can see the payload passes msg.sender as receiving address on other chain assuming that user have same address across all the evm chains, which is not the case if user is using the account abstraction wallet

    bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);

and than call the following function passing the payload, which calls the callContract function passing the payload to axelar network

  function _payGasAndCallCotract(
    string calldata destinationChain,
    string memory destContract,
    bytes memory payload
  ) private {
    GAS_RECEIVER.payNativeGasForContractCall{value: msg.value}(
      address(this),
      destinationChain,
      destContract,
      payload,
      msg.sender
    );

    // Send all information to AxelarGateway contract.
    AXELAR_GATEWAY.callContract(destinationChain, destContract, payload);
  }
  

Than on the destination any axelar node will call the excute() function passing in the payload and the tokens will be minted to account abstraction wallet address of the source chain but on destination same person will not be the owner of that address, and hence tokens are permanently lost

  function _execute(
    string calldata srcChain,
    string calldata srcAddr,
    bytes calldata payload
  ) internal override whenNotPaused {
  
    (bytes32 version, address srcSender, uint256 amt, uint256 nonce) = abi
      .decode(payload, (bytes32, address, uint256, uint256));

    if (version != VERSION) {
      revert InvalidVersion();
    }
    if (chainToApprovedSender[srcChain] == bytes32(0)) {
      revert ChainNotSupported();
    }
    // each chain have only on approved sender that is the source bridge contract.
    if (chainToApprovedSender[srcChain] != keccak256(abi.encode(srcAddr))) {
      revert SourceNotSupported();
    }
    if (isSpentNonce[chainToApprovedSender[srcChain]][nonce]) {
      revert NonceSpent();
    }

    isSpentNonce[chainToApprovedSender[srcChain]][nonce] = true;

    // same payload would have the same txhash
    bytes32 txnHash = keccak256(payload);
    txnHashToTransaction[txnHash] = Transaction(srcSender, amt);
    _attachThreshold(amt, txnHash, srcChain);
    _approve(txnHash);
    _mintIfThresholdMet(txnHash);
    emit MessageReceived(srcChain, srcSender, amt, nonce);
  }

Tools Used

Manual reviewing with some googling on different addresses across chains and have also read already somewhere in some contest, just could not find that report now.

Recommended Mitigation Steps

Give the user to pass in the address the tokens should be minted to on the destination bridge. And pass in the warning for account abstraction wallet holders to not to pass the same wallet. Some wallets may follow the deterministic deployment approach to have same address, but as safe explains that grantees nothing as each chain have its own different state and opcode differences so even deterministic approach may generate different addresses.

Assessed type

Invalid Validation

NC 1 : Use the uniswap multicall implementation that returns that exact error code, instead of the error code of multicall contract itself

Ondo use the following implementation of multicall in factory and source bridge:

  function multiexcall(
    ExCallData[] calldata exCallData
  ) external payable override onlyOwner returns (bytes[] memory results) {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);
      require(success, "Call Failed");
      results[i] = ret;
    }
  }

which simply returns the Call Failed error on !success, but use the uniswap impementation which returns the exact error code. Implementation is as follow: https://github.com/Uniswap/v3-periphery/blob/b13f9d9d9868b98b765c4f1d8d7f486b00b22488/contracts/base/Multicall.sol#L11-L28

    function multicall(bytes[] calldata data) public payable override returns (bytes[] memory results) {
        results = new bytes[](data.length);
        for (uint256 i = 0; i < data.length; i++) {
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);

            if (!success) {
                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
                if (result.length < 68) revert();
                assembly {
                    result := add(result, 0x04)
                }
                revert(abi.decode(result, (string)));
            }

            results[i] = result;
        }
    }
}

NC 2 : No need to use this much assembly in _rpow

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/rwaOracles/RWADynamicOracle.sol#L345-L398 _rpow function us copied from the following repositery,

https://github.com/makerdao/dss/blob/master/src/jug.sol

which is written in very old solidity version 0.6.12 when there was no native way to take power in solidity but now that is not the case.

So as the _rpow represents the following formula given by dev:

result = (prevRangeClose * [(numerator/ ONE) ** (elapsedDays + 1)])

This can be easily done using simple solidity instead use that.

NC 3: Use the mul functions copied as it is

_rmul and _mul function have been copied from following file:

https://github.com/makerdao/dss/blob/master/src/jug.sol

But developers have split it instead for which there is no need to do so, instead use it as or not use it at all. Making changing is susceptible to error

NC 4 : No need for underflow check in _mul function

The codebase from where the following function is copied

  function _mul(uint256 x, uint256 y) internal pure returns (uint256 z) {
    require(y == 0 || (z = x * y) / y == x);
  }

is written in old solidity version when there were no checks for the underflow or overflow, but with solidity 0.8.0 and later that is not the case. we can see the _mul make such simple multiplication a complete headache with those unnecessary checks which are already checked by in solidity version being used which is 0.8.16

NC 4 : Every role is granted to guardian, there always should be the separation of concern

Grant the following roles to different addresses:

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L134-L147

  function __rUSDY_init_unchained(
    address _usdy,
    address guardian,
    address _oracle
  ) internal onlyInitializing {
    usdy = IUSDY(_usdy);
    oracle = IRWADynamicOracle(_oracle);
    _grantRole(DEFAULT_ADMIN_ROLE, guardian);
    _grantRole(USDY_MANAGER_ROLE, guardian);
    _grantRole(PAUSER_ROLE, guardian);
    _grantRole(MINTER_ROLE, guardian);
    _grantRole(BURNER_ROLE, guardian);
    _grantRole(LIST_CONFIGURER_ROLE, guardian);
  }

NC 5: Unnecessary check in decrease allowance in rUSDY.sol

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L353-L364

  function decreaseAllowance(
    address _spender,
    uint256 _subtractedValue
  ) public returns (bool) {
    uint256 currentAllowance = allowances[msg.sender][_spender];
    require(
      currentAllowance >= _subtractedValue,
      "DECREASED_ALLOWANCE_BELOW_ZERO"
    );
    _approve(msg.sender, _spender, currentAllowance - _subtractedValue);
    return true;
  }

here no need for the check it will throw underflow check anyways

Low 1: If the oracle returned zero price ever the protocol is doomed.

oracle is an external factor that cannot be always trusted, and in this case the impact is too much. If the oracle returned the zero price, balances and shares in the rUSDY.sol become zero:

  function balanceOf(address _account) public view returns (uint256) {
    return (_sharesOf(_account) * oracle.getPrice()) / (1e18 * BPS_DENOMINATOR);
  }

  function totalSupply() public view returns (uint256) {
    return (totalShares * oracle.getPrice()) / (1e18 * BPS_DENOMINATOR);
  }

  function getSharesByRUSDY(
    uint256 _rUSDYAmount
  ) public view returns (uint256) {
    return (_rUSDYAmount * 1e18 * BPS_DENOMINATOR) / oracle.getPrice();
  }

  /**
   * @return the amount of rUSDY that corresponds to `_shares` of usdy.
   */
  function getRUSDYByShares(uint256 _shares) public view returns (uint256) {
    return (_shares * oracle.getPrice()) / (1e18 * BPS_DENOMINATOR);
  }

Description

rUSDY is the rebasing variant of USDY whose supply will be increased or decreased based upon the price returned from the custom oracle set up by the ondo team.

With this audit ondo also introduced the bridging mechanism to send tokens from one source to another to increase the usability of chain across chains for which ondo utilised the axelar network which have vast network of nodes to help process such cross chain transactions and transfers.

The key contracts of the protocol for this Audit are:

• SourceBridge.sol: This contract help in sending the tokens to another chain using the axelar gateway callContract function

• DestinationBridge.sol: This contract is axelar executable contract that exposes the execute function that can only be called by the nodes registered with the axelar and than mints the token to the desired address.

• MintRateLimiter.sol: This contract is used by destination bridge to limit the number of tokens minted on the destination per unit of time

• rUSDYFactory.sol: This contract is used to deploy the transparent upgradable proxy contracts and implementation contract using factory pattern.

• rUsDY.sol: This is the main token contract that is rebasing version of the USDY, user can call the wrap and unwrap function on this contract to convert their USDY back and forth.

Existing Patterns: The protocol uses standard Solidity and Ethereum patterns. It uses the ERC20 standards for most part (not completely though) and Accesibility pattern to grant different roles and also there is whitelisting and pausing mechanism too.

Approach

I took the top to down approach for this audit, so general flow is as follow

1. SourceBridge.sol
2. MintRateLimiter.sol
3. DestinationBridge.sol
4. rUSDY.sol
5. rUSDYFactory.sol

Reviewed the source bridge than destination and limiter side by side. As they were all very connected and certain checks needed to make sure in the flow. rUSDY is highly inspired by sthETH only few changes on how it calculates balance and mint share but for other part it was almost the same contract, which i think is a good approach given the amount of audits Lido did, there is alot of innate security guranteet.

Codebase Quality

I would say the codebase quality is good but can be improved, there are checks in place to handle different roles, each standard is ensured to be followed. And if something is not fully being followed that have been informed. But still it can be improved in following areas

Systematic and Centralisation Risks

1. Centralization Risk:

It is important to note that there are some previliged function in the system that if went rogue can cause significant problems for the protocol, for example all the roles being assigned only to the one guradian address on deployment. That is unnecessary there should always be some separation fo concern.

Than there are rescue functions in destination bridge and one another contract that decrease the credibility overall.

2. Systematic Risk

Once of the systematic risk is that protocol is highly dependent upon the axelar network which is not the best cross chain solution. Many cross chain hacks have occurred in past on these bridges.

Better options are:

1. Layer zero. 2, New chainlink CCIP

Both of them are very well established, and specifically the layer zero have done multiple security reviews and too solid at this point as compared to axelar. Instead use that.

Conclusion

Overall ondo came up with a very strong solution with some weak points in commenting and less docs and some centralisation risk. But the approach used for the core working of protocol is really solid and up to the industry standard and fixing above recommendation will make it even more robust.

Time spent:

20 hours

Time spent:

20 hours