Ondo Finance - Udsen's results

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L108-L109 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L137-L140 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L90-L91

Vulnerability details

Impact

The DestinationBridge._execute is an internal function that is executed when contract is called by Axelar Gateway. The _execute function stores the Transaction struct in the txnHashToTransaction mapping as shown below:

bytes32 txnHash = keccak256(payload);
txnHashToTransaction[txnHash] = Transaction(srcSender, amt); 

The transaction hash txnHash is calculated by keccak256(payload) and the payload is an abi encoded value consisting of following variables.

bytes32 version, address srcSender, uint256 amt, uint256 nonce

The issue here is that the two different srcChains with two different srcAddr contracts can end up providing the same txnHash if the above mentioned version, srcSender, amt and nonce are the same. The _execute function only restricts the same srcAddr to not to use the same nonce as shown below:

if (isSpentNonce[chainToApprovedSender[srcChain]][nonce]) {
  revert NonceSpent();
}

But the problem is if there are different srcAddr providing the same payload it will result into the same txnHash.

Hence there could be two transactions with the same transaction hash (txnHash). Hence the later transaction will override the txnToThresholdSet[txnHash] of the former transaction. As a result the approval process for transaction minting will be broken.

Proof of Concept

    bytes32 txnHash = keccak256(payload);
    txnHashToTransaction[txnHash] = Transaction(srcSender, amt);

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L108-L109

        txnToThresholdSet[txnHash] = TxnThreshold(
          t.numberOfApprovalsNeeded,
          new address[](0)
        );

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L137-L140

    (bytes32 version, address srcSender, uint256 amt, uint256 nonce) = abi
      .decode(payload, (bytes32, address, uint256, uint256));

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L90-L91

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to include the srcChain and the srcAddr in the payload as well which is getting hashed to calculate the txnHash. By doing so different transactions coming from different srcChains and srcAddr will not result into the same txnHash. Hence the approval process for transaction minting via the bridge will successfully execute.

Assessed type

Other

1. txnToThresholdSet[txnHash] RECORD HAS NOT BEEN DELETED AFTER THE TRANSACTION HAS BEEN MINTED

The DestinationBridge._mintIfThresholdMet function is used to mint a transaction if it has passed the threshold for the number of approvers. If the particular transaction with the provided txnHash has fulfilled the required approval threshold then the respective transaction will be minted and the record for the particular txnHash in the txnHashToTransaction mapping will be deleted as shown below:

  TOKEN.mint(txn.sender, txn.amount);
  delete txnHashToTransaction[txnHash];

But the issue here is that the txnToThresholdSet[txnHash] record for this particular txnHash which was minted, is not deleted.

Hence it is recommended to delete the txnToThresholdSet[txnHash] record in the DestinationBridge._mintIfThresholdMet function after the transactino has been minted as shown below:

  delete txnHashToTransaction[txnHash]; 
  delete txnToThresholdSet[txnHash];       

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L350

2. IT IS RECOMMENED TO FOLLOW THE CEI PROCESS WHEN CALLING EXTERNAL FUNCTIONS TO TRANSFER FUNDS

The rUSDY.wrap function is used by the users to wrap thier USDY into rUSDY. The function mint shares to the msg.sender and receives the USDY tokens from the msg.sender as shown below:

_mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR);
usdy.transferFrom(msg.sender, address(this), _USDYAmount);

But it does not follow the CEI flow. Hence it is recommended to mint the shares to the msg.sender after the USDY has been transferred from the msg.sender to the rUSDY contract.

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L436-L437

3. THE CRITICAL FUNCTION STATE CHANGES DO NOT HAVE EVENTS EMITTED WITH OLD AND NEW VALUES

The rUSDY.setOracle, rUSDY.setBlocklist, rUSDY.setAllowlist and rUSDY.setSanctionsList are called by the LIST_CONFIGURER_ROLE user to set critical addresses of the procotol. But none of these functions emit events.

Hence it is recommended to emit events for these critical functions by including both old and newly updated values, for future reference (logging).

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L662-664 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L698-L702 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L709-L713 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L720-L724

4. STATE VARIABLES ARE SHADOWED BY INPUT VARIABLE NAMES IN CRITICAL FUNCTIONS

The input variables to critical functoins such as rUSDY.setBlocklist, rUSDY.setAllowlist and rUSDY.setSanctionsList are overshadowing the state varaibles of inherited contracts. Hence it is recommended to change the names of input variables of above functions to avoid this issue.

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L701 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L712 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L723

5. LACK OF address(0) CHECK IN THE DestinationBridge.addApprover FUNCTION

The DestinationBridge.addApprover function is used to add an ondo Signer or Axelar Relayer to the approvers mapping. But there is no input validation for the passed in address approver input variable.

Hence it is recommended to perform the address(0) check on the address approver variable in the DestinationBridge.addApprover function.

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L210-L213

6. RWADynamicOracle.getPrice FUNCTION CAN BE MODIFIED TO INCREASE ITS EFFICIENCY DURING EXECUTION

The RWADynamicOracle.getPrice function is used to return the daily price of USDY given the range previously set. The getPrice function uses a for loop to iterate through the ranges array to derive the price based on the current block.timestamp as shown below:

for (uint256 i = 0; i < length; ++i) {
  Range storage range = ranges[(length - 1) - i];
  if (range.start <= block.timestamp) {
    if (range.end <= block.timestamp) {
      return derivePrice(range, range.end - 1);
    } else {
      return derivePrice(range, block.timestamp);
    }
  }
}

Above for loop can be modified as follows to increase the efficiency of its execution, by reducing the number of times if conditional statements are executed.

for (uint256 i = 0; i < length; ++i) {
  Range storage range = ranges[(length - 1) - i];
  if (range.end <= block.timestamp) {
    return derivePrice(range, range.end - 1);
  } else if (range.start <= block.timestamp) {
    return derivePrice(range, block.timestamp);        
  }
}

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L77-L86