Ondo Finance - 0xDING99YA's results

Lines of code

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

Vulnerability details

Impact

If a user send token from Chain A to Chain C, and during the waiting for approval the user send the same amount of token from Chain B to Chain C, if Chain A and Chain B have the same nonce, users can lost their token from Chain A to Chain C forever.

Proof of Concept

_execute() function in DestinationBridge.sol record a transaction from a source chain by txnHash, and txnHash is keccak256(payload). payload is just a combination of VERSION, sender, amount and nonce. So if a user send transactions from two source chains which have the same nonce, the txnHash will be exactly same. Since when a transaction is received it need to wait for approval to complete, if at this time a transaction with same txnHash is sent to the same destination chain, it can overwrite the pending transaction, and user will lose that token forever.

A POC is here to illustrate this, simply add it to forge-tests/bridges/DestinationBridge.t.sol and run

function test_User_can_lost_money() public {
// We already add chain support for arbitrum in setUp(), now we add support for optimism
vm.prank(guardian);
destinationBridge.addChainSupport(
  "optimism",
  "0xce16F69375520ab01377ce7B88f5BA8C48F8D612"
);

// To better illustrate a new bridged transaction can overwrite the existing bridged transaction, we set the need of
// approval larger than 2
amounts.push(500e18);
numOfApprovers.push(5);

vm.startPrank(guardian);
destinationBridge.setThresholds(arb, amounts, numOfApprovers);
destinationBridge.setThresholds("optimism", amounts, numOfApprovers);
vm.stopPrank();

// First transaction is a bridged transaction from Arbitrum, Alice want to send 100e18 token
bytes memory payload = abi.encode(bytes32("1.0"), alice, 100e18, 1);
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);

// ondoSigner0 also approves this arbitrum transaction, so the total approval should be 2 now
vm.prank(ondoSigner0);
destinationBridge.approve(keccak256(payload));
assertEq(destinationBridge.getNumApproved(keccak256(payload)), 2);

// Now while Alice's arbitrum transaction is waiting for approval, there is a exactly same transaction from optimism
cmdId = bytes32(
  0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e0
);
srcChain = "optimism";
srcAddress = "0xce16F69375520ab01377ce7B88f5BA8C48F8D612";

approve_message(
  cmdId,
  srcChain,
  srcAddress,
  address(destinationBridge),
  keccak256(payload)
);

destinationBridge.execute(cmdId, srcChain, srcAddress, payload);

// Now the new optimism overwrites the pending arbitrum transaction! We can see the transaction is overwritten
// and the approval for the same txnHash changes from 2 to 1
assertEq(destinationBridge.getNumApproved(keccak256(payload)), 1);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Add srcChain when creating a payload, this can prevent two source chains have exactly same payload.

Assessed type

Other