Ondo Finance - 0xpiken's results

Lines of code

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

Vulnerability details

Impact

It is very common that user use multi-signature smart contract wallet to manage their digital assets for safety. Gnosis Safe is a multi-signature wallet and it is widely used.

Unlike EOA wallet, user owns a smart-contract wallet in one chain doesn't mean they own same smart-contract wallet in other chains.

When generating payload in SourceBridge.sol, msg.sender is assumed to be the receiver in destination chain to receive minted token, but the address in destination chain may not exist or controlled by malicious one if smart contract wallet is used, and all minted token could be locked forever or stolen by malicious attacker.

Check https://mirror.xyz/0xbuidlerdao.eth/lOE5VN-BHI0olGOXe27F0auviIuoSlnou_9t3XRJseY to find out how 20 million $OP got stolen.

Proof of Concept

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L79 msg.sender is encoded in payload and sent to destination chain. https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L90-L91 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L109 msg.sender is decoded from payload and stored as txnHashToTransaction[txnHash].sender https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L349 token is minted to txnHashToTransaction[txnHash].sender aka msg.sender.

If msg.sender is a smart-contract address, There is a high possibility that all bridged tokens are locked forever or stolen by malicious user because those who bridge tokens may not have privilege to access it.

Tools Used

Manual Review

Recommended Mitigation Steps

Add parameter address receiver in SourceBridge#burnAndCallAxelar() to let user choose which address in destination chain they want to bridge. receiver should be encoded in payload and decoded from payload in DestinationBridge#_execute()

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When user bridges same amount of tokens from different chains to the same destination chain, there is a possibility that txnHash could be duplicated. In this case the previous transaction and thresholds may be rewrote and only the last one can be executed successfully, which means user may only receive one amount of tokens in destination chain and the rest will be lost forever.

Proof of Concept

Alice owns some TOKEN respectively in the same EOA address on A chain and B chain. 1.Alice bridge 1 TOKEN from A chain to C chain.(Transaction 1) 2.Alice bridge 1 TOKEN from B chain to C chain at the same time. (Transaction 2) Suppose any transaction no more than 10 TOKEN needs 3 approvals. it could happen as below: Transaction 1 is stored after DestinationBridge#_execute() is executed successfully. Bridged token in Transaction 1 is not minted to receiver because it needs two more approvals. At this time transaction 2 is triggered, by chance it has the same nonce in payload with transaction 1, transaction will be erased and replaced to transaction 2 since calculated txnHashs are same in two transactions because of the same payload is used for calculation. Only 1 TOKEN in transaction 2 will be minted to receiver. the other 1 TOKEN will be lost forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Calculate txnHash using the combination of srcChain, srcAddress and payload:

 bytes32 txnHash = keccak256(abi.encode(srcChain, srcAddress, payload));

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the addresses of SourceBridge smart contracts in different chains are same, conflicts will occur and Most of bridging transactions from these chains will be reverted.

Proof of Concept

1. SourceBridge was deployed on A chain with address 0x1A...6C
2. SourceBridge was deployed on B chain with address 0x1A...6C, same as on A chain.
3. One bridging transaction was executed from A chain , supposed nonce is 0: isSpentNonce[chainToApprovedSender[srcChain]][nonce], aka isSpentNonce[keccak256(abi.encode(0x1A...6C))][0] was set to true
4. Another bridging transaction was executed from B with nonce 0. Since isSpentNonce[keccak256(abi.encode(0x1A...6C))][0] has been set to true, this bridging transaction will be reverted because the address of SourceBridge in B chain is same as the address of SourceBridge in A chain.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the code as below:

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();
    }
    //@audit-info change the line below
    if (isSpentNonce[keccak256(abi.encode(srcChain, chainToApprovedSender[srcChain]))][nonce]) {
         revert NonceSpent();
    }
    //@audit-info change the line below
    isSpentNonce[keccak256(abi.encode(srcChain, chainToApprovedSender[srcChain]))][nonce] = true;
    bytes32 txnHash = keccak256(payload);
    txnHashToTransaction[txnHash] = Transaction(srcSender, amt);
    _attachThreshold(amt, txnHash, srcChain);
    _approve(txnHash);
    _mintIfThresholdMet(txnHash);
    emit MessageReceived(srcChain, srcSender, amt, nonce);
}

Assessed type

Other