Ondo Finance - peanuts's results

Lines of code

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

Vulnerability details

Impact

There must always be more than 0 approvals to withdraw tokens, which breaks protocol logic. In the case of setting 0 approvals, user will not be able to withdraw their funds.

Proof of Concept

In setThresholds(), there is a possibility that there is no need for any approvals due to this comment:

   * @dev This function will remove all previously set thresholds for a given chain
   *      and will thresholds corresponding to the params of this function. Passing
   *      in empty arrays will remove all thresholds for a given chain
   */

If that happens, the execute() function fails to work because of the _attachThreshold() function. In _attachThreshold(), if the number of approvals is zero, the function reverts.

    if (txnToThresholdSet[txnHash].numberOfApprovalsNeeded == 0) {
      revert NoThresholdMatch();
    }

This means that in setThresholds(), the numOfApprovers array must always be more than zero, which means that the amounts array must be more than zero. There can never be a case when there is zero approvals.

  function setThresholds(
    string calldata srcChain,
    uint256[] calldata amounts,
    uint256[] calldata numOfApprovers
  ) external onlyOwner {
    if (amounts.length != numOfApprovers.length) {
      revert ArrayLengthMismatch();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The check in _attachThreshold() needs to be sidestepped if the protocol intends to not have any approval. Probably have another function that has a boolean value that can be toggled by the owner. If toggled, then the function below will not run.

    if (txnToThresholdSet[txnHash].numberOfApprovalsNeeded == 0) {
      revert NoThresholdMatch();
    }

Assessed type

Context

1. Any comments for the judge to contextualize your findings

• Focused on the rUSDY and Destination Bridge contract, and followed through with all the test files for those contracts

2. Approach taken in evaluating the codebase

• Understood the relation between USDY,rUSDY and shares first, then check all the functions in rUSDY to make sure that they are working as intended and cannot be exploited easily
• Run the test files of rUSDY and run some fuzz testing to make sure that wrap() and unwrap() works as intended with different time stamp and oracle price
• Moved on to the source and destination bridge and ran the tests
• Made sure that the test are properly written and focused on the setThresholds() function in Destination contract
• Zoomed out and looked at all the contracts as a whole to check whether all contracts and imports are written correctly, and all contracts have the pause function as well as being protected by the modifier.

3. Mechanism review

• I don't think that rUSDY can be called a rebasing token because it is not pegged to USDY but rather to the dollar. Also, the price of rUSDY is taken from the oracle of USDY, so it's more like a vault token than a rebasing token
• There is a part in the rUSDY contract that has a truncation to zero issue. If the shares passed into getRUSDYByShares() is less than 10,000 and the oracle returns the price of 1e18, then the price of RUSDYByShares will be zero. Minimal loss as only dust amounts will be lost

  function getRUSDYByShares(uint256 _shares) public view returns (uint256) {
    return (_shares * oracle.getPrice()) / (1e18 * BPS_DENOMINATOR);
  }

• In the Destination contract, there is a heavy emphasis on restricting users from minting the token on the destination chain. The restriction includes having the need to have approvals and having a rate limiter with a maxTokenlimit

      _checkAndUpdateInstantMintLimit(txn.amount);

4. Centralization Risk

• The owner can burn the rUSDY of any account and pocket the USDY

  function burn(
    address _account,
    uint256 _amount
  ) external onlyRole(BURNER_ROLE) {
    uint256 sharesAmount = getSharesByRUSDY(_amount);
    _burnShares(_account, sharesAmount);
    usdy.transfer(msg.sender, sharesAmount / BPS_DENOMINATOR);

• Owner controls the approval and mint limit of the Destination contract as well

  function addApprover(address approver) external onlyOwner {
    approvers[approver] = true;
    emit ApproverAdded(approver);
  }

• Owner is able to pause and unpause all parts of the protocol

  function pause() external onlyOwner {
    _pause();
  }
  /**
   * @notice Admin function to unpause the contract
   *
   * @dev Only used for bridge functions
   */
  function unpause() external onlyOwner {
    _unpause();

• In summary, if owner turns malicious, the whole contract is at stake

Time spent:

14 hours