Ondo Finance - bin2chen's results

Lines of code

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

Vulnerability details

Impact

DestinationBridge.txnHash without adding srcChain to the hash calculation Resulting in the same value of txnHash for different chains, old transactions are overwritten, transactions cannot be executed, tokens are lost

Proof of Concept

txnHashis generated as follows:

  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));

    isSpentNonce[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);
  }  

From the above code, we know that txnHash is only related to version, srcSender, amt, nonce.

If the same user in a different chain has the same amt/nonce, txnHashToTransaction[txnHash] will be overridden causing older transactions to fail and tokens to be lost.

test poc

The following test code demonstrates different chains, only one transaction can be executed, the other transaction can not be executed, the token will be lost

add to DestinationBridge.t.sol

  function test_hash_conflict() public {
   
    vm.prank(guardian);
    destinationBridge.addChainSupport(
      "optimism",
      "0xce16F69375520ab01377ce7B88f5BA8C48F8D612"
    );

    bytes32 cmdId = bytes32(
      0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e0
    ); 
    bytes32 cmdIdOptimism =  bytes32(
      0x9991faa1f435675159ffae64b66d7ecfdb55c29755869a18db8497b4392347e1
    );  
    string memory srcChain = "arbitrum";
    string memory srcAddress = "0xce16F69375520ab01377ce7B88f5BA8C48F8D666";
    address srcSender = alice;
    uint256 amt = 100e18;
    uint256 nonce = 1;
    bytes memory payload = abi.encode(VERSION, srcSender, amt, nonce);

    approve_message(
      cmdId,
      srcChain,
      srcAddress,
      address(destinationBridge),
      keccak256(payload)
    );
    approve_message(
      cmdIdOptimism,
      "optimism",
      "0xce16F69375520ab01377ce7B88f5BA8C48F8D612",
      address(destinationBridge),
      keccak256(payload)
    );    
    //1. address(this) execute arbitrum
    destinationBridge.execute(cmdId, srcChain, srcAddress, payload);
    //2.alice execute optimism , avoid duplicate approve.
    vm.prank(alice);
    destinationBridge.execute(cmdIdOptimism, "optimism", "0xce16F69375520ab01377ce7B88f5BA8C48F8D612", payload);
    //3.approve arbitrum
    vm.prank(ondoSigner0);
    destinationBridge.approve(keccak256(payload));
    //4.approve other will revert
    vm.prank(ondoSigner0);
    destinationBridge.approve(keccak256(payload));    
  }  

$ yarn test-forge --match-test test_hash_conflict

Running 1 test for forge-tests/bridges/DestinationBridge.t.sol:Test_DestinationBridge
[FAIL. Reason: RateLimit: mint amount can't be zero] test_hash_conflict() (gas: 438665)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 12.14ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in forge-tests/bridges/DestinationBridge.t.sol:Test_DestinationBridge
[FAIL. Reason: RateLimit: mint amount can't be zero] test_hash_conflict() (gas: 438665)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Recommended Mitigation Steps

add srcChain into txnHash generating

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

Assessed type

Context

L-01: _mintIfThresholdMet() A malicious ALLOWLIST can duplicate mint

_mintIfThresholdMet does not follow the Checks Effects Interactions principle If ALLOWLIST is a malicious contract, it is possible to repeat the mint token It is recommended to clear before executing mint

  function _mintIfThresholdMet(bytes32 txnHash) internal {
    bool thresholdMet = _checkThresholdMet(txnHash);
    Transaction memory txn = txnHashToTransaction[txnHash];
    if (thresholdMet) {
+     delete txnHashToTransaction[txnHash];    
      _checkAndUpdateInstantMintLimit(txn.amount);
      if (!ALLOWLIST.isAllowed(txn.sender)) {
        ALLOWLIST.setAccountStatus(
          txn.sender,
          ALLOWLIST.getValidTermIndexes()[0],
          true
        );
      }
      TOKEN.mint(txn.sender, txn.amount);
-     delete txnHashToTransaction[txnHash];
      emit BridgeCompleted(txn.sender, txn.amount);
    }

L-02: transferFrom() proposes to ignore allowances if _sender==msg.sender.

The current transferFrom() still requires an allowance if _sender==msg.sender. Some protocols that transfer out tokens use transferFrom() instead of transfer(). Suggest adding _sender==msg.sender to ignore allowances to accommodate these protocols

L-03: transferFrom() Malicious Allowlist contract can reuse user's allowances

transferFrom() does not follow the Checks Effects Interactions principle

Currently it would be the following steps

1. _transfer() -> _beforeTokenTransfer() -> _isAllowed() -> allowlist.isAllowed
2. _approve(_sender, msg.sender, currentAllowance - _amount)

This way if the allowlist is maliciously contracted, you can reenter transferFrom() to reuse the allowance

Suggestion, modify allowances first, before executing _transfer().

  function transferFrom(
    address _sender,
    address _recipient,
    uint256 _amount
  ) public returns (bool) {
    uint256 currentAllowance = allowances[_sender][msg.sender];
    require(currentAllowance >= _amount, "TRANSFER_AMOUNT_EXCEEDS_ALLOWANCE");
+   _approve(_sender, msg.sender, currentAllowance - _amount);
    _transfer(_sender, _recipient, _amount);
-   _approve(_sender, msg.sender, currentAllowance - _amount);
    return true;
  }

L-04: BURNER_ROLE can't burn off blacklisted user's token

Administrators with BURNER_ROLE privileges can execute burn() to burn a user's shares and transfer usdy to themselves.

Currently execute burn() -> _burnShares() -> _beforeTokenTransfer() -> _isBlocked() -> blocklist.isBlocked()

This way, if the user is blacklisted, the administrator burn is unable to execute the

Suggestion

_beforeTokenTransfer() no longer checks _isBlocked/_isSanctioned/_isAllowed if msg.sender has BURNER_ROLE privileges.

L-04: unwrap() leaves a small portion of each user in the contract

The current implementation of unwrap() is as follows.

 function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
    require(_rUSDYAmount > 0, "rUSDY: can't unwrap zero rUSDY tokens");
    uint256 usdyAmount = getSharesByRUSDY(_rUSDYAmount);
    if (usdyAmount < BPS_DENOMINATOR) revert UnwrapTooSmall();
@>  _burnShares(msg.sender, usdyAmount);
@>  usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);
    emit TokensBurnt(msg.sender, _rUSDYAmount);
  }

From the code above we know that usdy.transfer() rounds up usdyAmount But _burnShares() does not round it up This results in a mismatch between the user burn and the usdy obtained, out of the decimal difference

suggest _burnShares() does the rounding

 function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
    require(_rUSDYAmount > 0, "rUSDY: can't unwrap zero rUSDY tokens");
    uint256 usdyAmount = getSharesByRUSDY(_rUSDYAmount);
    if (usdyAmount < BPS_DENOMINATOR) revert UnwrapTooSmall();
-   _burnShares(msg.sender, usdyAmount);
+   _burnShares(msg.sender, usdyAmount / BPS_DENOMINATOR * BPS_DENOMINATOR);
    usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);
    emit TokensBurnt(msg.sender, _rUSDYAmount);
  }