Users may lose their token when send tokens cross chain

[code423n4]

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

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-sponsor]

tom2o17 marked the issue as disagree with severity

[tom2o17]

So agree with this issue,
I am curious given the hoops that a user must jump through to invoke this error if this can be downgraded.

Basically I feel that given a user must wait for the nonce to be the same between src chains and must pass the message using the same wallet and amount.

tldr: ilf the user really needs to put in work to invoke this error.

[c4-sponsor]

ypatil12 (sponsor) confirmed

[kirk-baird]

I'm inclined to accept this as high severity.

There are requirements for this attack / bug to have the same:

• address
• amount
• nonce

If these conditions are met there will be significant loss of funds.

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked issue #391 as primary and marked this issue as a duplicate of 391

[tom2o17]

@kirk-baird

Agree with the above more so a comment that the conditions to induce the bug are sufficiently difficult to produce, given that the user will have to be listening to cross chain state and be subject to race conditions, all so that the user ....checks notes.... burns their funds.

Generally think that when working with invalid state paths it is worth considering the difficulty of producing said bug in the wild, when weighting the severity, but defer to y'all. 🤠

[stalinMacias]

@kirk-baird
I think this issue fits more the judging description of a medium severity than a high severity. As explained by @tom2o17 , to me it looks like it matches the exact description of medium severity issue: "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements."

[0xnirlin]

Came to comment the same as stalin, the condition required to make this happen are not too straight forward.

[kirk-baird]

The debate here for this issue and new primary #391 is whether the likelihood of this issue is high enough to qualify for high severity vs medium severity.

Looking at each aspect

• amount:
  • an EOA user to send same amounts from multiple chains to a single destination chain
  • a smart contract allowing users to bridge funds (note this would require the equivalent address on the receiving chain, smart contracts are not the intended users for this protocol)
• address:
  • if account is an EOA then they own keys on both chains
  • smart contract would only occur if it is deployed to the same address on both chains
• nonce:
  • to be equal on both chains. Anyone can spam calls to burnAndCallAxelar() to increment the lower nonce such that they match. However, this requires front-running and organizing the mem pool so would realistically need to be a block producer to exploit this.
• timing: attacks can only be performed while txs are pending

I'm going to consider the opinions from the sponsor and other wardens here, that this is not sufficient likelihood to qualify for a high severity and downgrade it to medium.

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)