Ondo Finance - ast3ros's results

Lines of code

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

Vulnerability details

Impact

The bridge contracts assume that the source and destination chains have the same address encoding, which may not be true and cause the users to lose their tokens in the destination chain.

Proof of Concept

In SourceBridge, the source sender is set to msg.sender and encoded into the payload.

    bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);

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

In destination chain, the DestinationBridge will mint the new tokens to the same address as the source sender.

    TOKEN.mint(txn.sender, txn.amount);

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

This will work fine if the bridges are only used in the chains that are EVM compatible and have the same address encoding.

However, if the bridges are used in the chains that are EVM compatible but have different address encoding, the users will not receive the tokens in the destination chain because there may be no address that matches the source sender in the destination chain.

For example, TRON is EVM compatible but has different address encoding from Ethereum. If the bridges are used in Ethereum and TRON, the users will not receive the tokens in TRON. Currently, TVM is compatible with EVM.

https://tronprotocol.github.io/documentation-en/contracts/tvm/#features-of-tvm: https://stackoverflow.com/questions/74974850/creating-a-wallet-on-ether-creates-it-on-all-evn-supported-networks

Because the contracts are deployed to any EVM compatible chains, it is possible that the bridges are used in the chains that have different address encoding.

Tools Used

Manual

Recommended Mitigation Steps

Allow users to set receiver address instead of using msg.sender address from source chain as the receiver address.

Assessed type

en/de-code

Lines of code

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

Vulnerability details

Impact

The destination bridge does not distinguish the source chain of the transactions, which may cause nonce collision and revert the minting of the bridged tokens. The destination bridge will revert some valid transactions because the nonce is already spent.

Proof of Concept

In the destination bridge, the nonce is checked to ensure that no transaction is replayed. The isSpentNonce is the mapping of hash of approved sender => nonce => bool and set to true when the transaction is executed.

    mapping(bytes32 => mapping(uint256 => bool)) public isSpentNonce; 

The problem is that the mapping does not specify which chain the transaction is from. Therefore, if two transactions from different chains have the same approved sender address and the same nonce, the second transaction will fail because the nonce is already spent.

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

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

This can happen because the address of the contract is defined by the address of the deployer and nonce. If the deployer address is the same in both chains, the address of the contract can be the same.

The second transaction failure will cause the user to lose their bridged tokens because the minting is reverted and the tokens are already burnt in the source chain

    TOKEN.burnFrom(msg.sender, amount);

https://github.com/code-423n4/2023-09-ondo/blob/b88271d64112234b7a7273cd7f3cea73c350e6a7/contracts/bridge/SourceBridge.sol#L77

Tools Used

Manual

Recommended Mitigation Steps

The isSpentNonce should be specified by which chain the transaction is from.

For example, the mapping can be changed to mapping(bytes32 => mapping(uint256 => mapping(uint256 => bool))) public isSpentNonce; where the first uint256 is the chain id.

Assessed type

Other

[L-1] PausableUpgradeable contract is not initialized

Contract that inherit from PausableUpgradeable should call the __Pausable_init() function in their initialize function. This is not done in the rUSDY contract.

https://github.com/code-423n4/2023-09-ondo/blob/b88271d64112234b7a7273cd7f3cea73c350e6a7/contracts/usdy/rUSDY.sol#L128-L131

Recommended Mitigation Steps:

    ) internal onlyInitializing {
        __BlocklistClientInitializable_init(blocklist);
        __AllowlistClientInitializable_init(allowlist);
        __SanctionsListClientInitializable_init(sanctionsList);
        __rUSDY_init_unchained(_usdy, guardian, _oracle);
+       __Pausable_init();        
    }

[L-2] Number of approve needed is always minus 1

In the DestinationBridge, the number of approvals needed is always one less than the threshold. This is because there is an automatic approval when _execute function is called.

    _approve(txnHash);

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

This means that the number of approvals needed is always reduced by one. For example, if the threshold is 2, then the number of approvals needed is 1. If the threshold is 3, then the number of approvals needed is 2.

[NC-1] Unnecessary overflow checking

In _rmul function, the x * y operation is executed in the _mul(x, y) function. The _mul function performs the overflow checking by require(y == 0 || (z = x * y) / y == x);. However since solidity 0.8, overflow checking is done by default. Therefore, the overflow checking in _mul function is unnecessary and can be removed.

https://github.com/code-423n4/2023-09-ondo/blob/b88271d64112234b7a7273cd7f3cea73c350e6a7/contracts/rwaOracles/RWADynamicOracle.sol#L405-L407

[NC-2] State is not cleared for the executed transaction

In the destination bridge, after the transaction is executed, not all the state variables related to the transaction are cleared. There are two state variables:

• txnHashToTransaction is cleared
• txnToThresholdSet is not cleared

Gas can be saved by clearing the txnToThresholdSet state variable.

[NC-3] Inaccurate variable name

In the unwrap function, the usdyAmount is not the amount of USDY to be unwrapped, but the amount of shares to be burned. Therefore, the variable name should be changed to sharesAmount instead of usdyAmount to avoid confusion. It can mislead the reader to think that the amount of USDY.

https://github.com/code-423n4/2023-09-ondo/blob/b88271d64112234b7a7273cd7f3cea73c350e6a7/contracts/usdy/rUSDY.sol#L451