Funds will be lost in case the block reorg occurs on the chain of sending bridge

[c4-submissions]

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

Funds will be lost in case the block reorg occurs on the chain of sending bridge

Proof of Concept

consider the following scenerio on sending chain:

In block 1: Alice sends the 1000 tokens to the destination bridge and the nonce for it is set to 10
In block 2: Bob sends the 1000 tokens to the destination bridge and the nonce for it is set to 11
In block 3: Alice again sends the 1000 tokens to the destination bridge and the nonce for it is set to 12

Not the events have been transmitted and axelar network have started its work.

Than the block reorg happens and block2 along with block 3 is orphaned. Which means the nonce gone.

Now we have block 1 on which alice called the transaction with nonce 10. And we have the nonce 11 and 12 on our hand again.

So in the upcoming blocks the axelar network will be again called with the nonce of 11 and 12 which have been processed on the receiving chain and have minted tokens to alice and bob.

lets say reorg happened on bnb chain than the following in _execute() will be set to true for nonce 11 and 12 and will revert next time :

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

Now lets say two new user adam and eve sends the tokens in next two function, with nonce of 11 and 12.

This time _execute will revert due to the nonce being used before for the source chain, as destination is not aware of the block reorg.

As discussed in other issues that, if the _execute reverts once, the transaction is unplayable again and the tokens burnt on source chain are lost forver. Hence loss of funds for adam and eve

 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 review and miltruck submissin from lens protocol.

Recommended Mitigation Steps

Really not sure what could be done here, will need to discuss this one with few fellows.

Assessed type

Other

[raymondfam]

QA at best based on C4 recent contests.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[0xnirlin]

Respected judge:
Considering the recent judging about bloc reorg, i submitted this as medium. For example look at the following issue:

code-423n4/2023-07-lens-findings#148

Can you give this issue a second look please, I respect the original decision and respect it if you decide to keep it as it is.

Thank you.

[stalinMacias]

@AhmadDecoded So, the core argument of your report is that a reorg could occur in the source chain and the nonces would be modified, right? I think it's important to consider the fact that it is Axelar's duty to enforce a correct validation mechanism to prevent this type of problem when they process the crosschain message. For example, it is axelar's responsibility to enforce a correct threshold for when a crosschain message should be executed, part of this is to consider the # of blocks since the event was emitted, let's say, in Polygon axelar might need to configured a parameter that validates that at least, let's say, 120 blocks have been validated since the message was emitted, and for Arbitrum it might need to wait for only 60 blocks, but ultimately, preventing issues caused by reorgs falls more into the axelar's domain than the protocol itself.

But feel free to prove me wrong and demonstrate that the reorg is something that the protocol must be responsible for

[kirk-baird]

Agreed with @stalinMacias that re-orgs are handled by Axelar bridge by enforcing a significant delay. If re-orgs are not handled by Axelar bridge then anyone can game the bridge and it is not useful.

There will be significant delay between source chain creating the nonces and them being executed on the destination chain. If a long enough re-org occurs so that source chain is emitting different messages then this is an issue with the Axelar bridge and no Ondo.