Ondo Finance - 0xStalin's results

Lines of code

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

Vulnerability details

Impact

• The protocol could not enforce their policies over users who are not allowed to own USDY because the users can prevent the burning of their tokens by either unwrap their rUSDY or move it to different accounts.

Proof of Concept

• The purpose of the rUSDY:burn() can be thought of as a mean for the protocol to seize funds from accounts who are not allowed to own USDY (see sponsor's explanation).

• The problem with the existing logic is that the account must not be blacklisted/sanctioned and it has to be allowed, the reason is because to burn the shares, the _burnShares() is called, and in this function, the _beforeTokenTransfer() function is called, inside the _beforeTokenTransfer() is verified if the account is blacklisted/sanctioned or not allowed to execute operations.

  • So, if the account must be allowed in order for the burning operation to be executed, that means that the account's owner can also execute transfers of rUSDY to another accounts and unwrap the rUSDY for USDY.

    • burn() => _burnShares() => _beforeTokenTransfer()

function _beforeTokenTransfer(
  address from,
  address to,
  uint256
) internal view {
  // Check constraints when `transferFrom` is called to facliitate
  // a transfer between two parties that are not `from` or `to`.
  if (from != msg.sender && to != msg.sender) {
    require(!_isBlocked(msg.sender), "rUSDY: 'sender' address blocked");
    require(!_isSanctioned(msg.sender), "rUSDY: 'sender' address sanctioned");
    require(
      _isAllowed(msg.sender),
      "rUSDY: 'sender' address not on allowlist"
    );
  }

  //@audit-info => When burning, it will check if the account is not blacklisted/sanctioned and it is allowed, if the account meets this criteria, it also means that the owner can do transfers and unwrap rUSDY
  if (from != address(0)) {
    // If not minting
    require(!_isBlocked(from), "rUSDY: 'from' address blocked");
    require(!_isSanctioned(from), "rUSDY: 'from' address sanctioned");
    require(_isAllowed(from), "rUSDY: 'from' address not on allowlist");
  }

  if (to != address(0)) {
    // If not burning
    require(!_isBlocked(to), "rUSDY: 'to' address blocked");
    require(!_isSanctioned(to), "rUSDY: 'to' address sanctioned");
    require(_isAllowed(to), "rUSDY: 'to' address not on allowlist");
  }
}

• Let's suppose the scenario where the protocol has determined that AccountA is not allowed to own USDY, the account currently owns 250 USDY. So, the protocol calls the burn() to burn the 250 USDY(shares) from the AccountA. In the mempool it is already a tx from the AccountA's owner to unwrap 50 USDY.
  • So, the tx of AccountA's owner is executed first and then is executed the tx from the protocol, as a result, the tx from the protocol to burn the 250USDY fails because now the AccountA only have 200USDY, the owner of the AccountA realizes that the protocol is trying to burn his USDY and proceeds to unwrap all the reamining balance or transfer it to different accounts.
  • The result is that the protocol was not able to seize the funds from an account that is not allowed to hold USDY.

Tools Used

Manual Audit

Recommended Mitigation Steps

• Refactor the logic in the _burnShares() so that depending on the reason to burn shares will check or not that the account is blacklisted/sanctioned.
• If the protocol has determined that an account is not allowed to hold funds and will proceed to seize them, then, that account should be either be blacklisted/sanctioned or notAllowed to perform any operation that would prevent the seizing of their tokens.
• Add a new flag to the _burnShares() that will indicate to the function if needs to check the account status (blacklisted/sanctioned) or not.
  • If the burning is coming from the burn(), the account check should not be done, and the burning should be executed anyways, but if the burning is coming from another function, like the unwrap(), then the account check should be done.

function burn(
  address _account,
  uint256 _amount
) external onlyRole(BURNER_ROLE) {
  uint256 sharesAmount = getSharesByRUSDY(_amount);

  //@audit-info => Set to true the seizing flag, so the account check is skipped, because the account should already be blacklisted/sanctioned!
  _burnShares(_account, sharesAmount,true);

  ...
}

function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
  ...

  //@audit-info => Set to false the seizing flag, so the account check is made!
  _burnShares(msg.sender, usdyAmount,false);
  
  ...

}


function _burnShares(
  address _account,
  uint256 _sharesAmount,
  bool seizing
) internal whenNotPaused returns (uint256) {
  ...

  //@audit-info => Check the account status if the burning comes from any function other than the burn() | If the flag is set to false, then, the account check must be done!
  if(!seizing) _beforeTokenTransfer(_account, address(0), _sharesAmount);

  ...

}

Assessed type

Timing

[L-01] Number of approvers can be set to be greather than the existing approvers in the DestinatinationBridge contract

When setting the tresholds in the DestinationBridge, there is no checks to validate if the number of approvers that are been set to the ranges surpasses the existing maximum number of appprovers in the contract. Setting the number of required approvers greather than the total number of approvers will cause that tx never reaches the approved status until new approvers are added to the DestinationBridge contract.

function setThresholds(
  string calldata srcChain,
  uint256[] calldata amounts,
  uint256[] calldata numOfApprovers
) external onlyOwner {
  if (amounts.length != numOfApprovers.length) {
    revert ArrayLengthMismatch();
  }
  delete chainToThresholds[srcChain];
  for (uint256 i = 0; i < amounts.length; ++i) {
    if (i == 0) {
      chainToThresholds[srcChain].push(
        Threshold(amounts[i], numOfApprovers[i])
      );
    } else {
      if (chainToThresholds[srcChain][i - 1].amount > amounts[i]) {
        revert ThresholdsNotInAscendingOrder();
      }
      //@audit-issue => Not validating if numOfApprovers[i] is greather than the existing number of approvers
      chainToThresholds[srcChain].push(
        Threshold(amounts[i], numOfApprovers[i])
      );
    }
  }
  emit ThresholdSet(srcChain, amounts, numOfApprovers);
}

Fix:

• Keep track of the total numbers of approvers, and when setting thresholds validate if the number of required approvers is greather than the total approvers, if so, either revert the tx or set the number of required approvers as the total number of approvers, or emit an event to notify off-chain that will be required to add more approvers to the DestinationBridge

[L-02] When attaching thresholds, if the amount of tokens to be minted exceeds all the thresholds, the tx will be reverted because that amount won't be assigned a number of required approvals!

When attaching thresholds in the DestinationBridge as part of the flow when running the _execute() that is called by the Axelar Gateway, if the amount of tokens to be minted exceeds all the existing thresholds, the numberOfApprovalsNeeded won't be updated, thus, the default value will be 0, which will cause the tx to be reverted.

function _attachThreshold(
  uint256 amount,
  bytes32 txnHash,
  string memory srcChain
) internal {
  Threshold[] memory thresholds = chainToThresholds[srcChain];
  for (uint256 i = 0; i < thresholds.length; ++i) {
    Threshold memory t = thresholds[i];
    if (amount <= t.amount) {
      txnToThresholdSet[txnHash] = TxnThreshold(
        t.numberOfApprovalsNeeded,
        new address[](0)
      );
      break;
    }
  }
  //@audit-info => If the amount exceeded all the thresholds, the numberOfApprovalsNeeded won't be updated, thus, the tx will be reverted!s
  if (txnToThresholdSet[txnHash].numberOfApprovalsNeeded == 0) {
    revert NoThresholdMatch();
  }
}

Fix: Use the last threshold for huge amounts of tokens to minted, instead of reverting the tx, assign a default number of approvers for all the amounts that exceeds a certain amount.

• Example, if the last threshold would be for 1000 tokens to be minted and require 5 approvers, add a new range at the end to prevent the tx if the amount exceeds the 1000 tokens, and set more number of approvers. If transactions are reverted because the amount exceeds all the thresholds, one of two things will happen, either the admins will need to update the thresholds, or the minting of those tokens will get stuck.

Analysis of the Ondo Finance Contest

Introduction

• The Ondo Finance protocol is introducing a rebasing token for their existing yielding token USDY.

  • The purpose of the rUSDY token is to make easier the process to transact since 1 rUSDY is worth 1 dollar, in comparisson, the value of the USDY token varies overtime, which that might cause some troubles when transacting and estimating costs.
  • Any user who hold USDY can wrap their tokens for rUSDY, the rUSDY balances will rebase as USDY accrues values.
  • User can unwrap their rUSDY and receive the equivalent of their balances in USDY based on the current price of USDY at the moment of unwrapping.
  • rUSDY calls into RWADynamicRateOracle.sol in order to fetch the current price of USDY and determine its worth in USD

• The RWADynamcRateOracle contract is used to post price evolution for USDY on chain

• In addition to the new rebasing token, the Ondo Protocol also introduces an implementation to bridge assets among different chains by using the Axelar Network.

  • Basically they are taking advantage of the security and infraestructure of the Axelar Network to perform the cross chain message communication.
  • Two contracts were created for this implementation, a SourceBridge and a DestinationBridge, both of these contracts will be deployed on each chain that will be supported, the bridges are multiconnected, meaning, one SourceBridge can send crosschain messages to different DestinationBridges, or in other words, each DestinationBridge can receive messages from the different SourceBridges.
  • This solution allows the Ondo Team to offer to their users the possibility to bridge their USDY holdings among different chains.

Analysis of the Codebase

• In general, the codebase is very hardened, I didn't find any high vulnerabilities that allows attackers to increase the supply of rUSDY, or steal other user's USDY wrapped tokens.

• Also, the bridge system is very robust, the protocol did a good work implementing their bridge solution with the Axelar Network. The Axelar Network is basically in charge of the cross chain communication layer, which allows the Ondo Protocol to bridge their USDY token among different chains.

• The oracle is designed to be updated manually, and the implementation correctly returns the requested price, which is required by the rUSDY contract to determine the value in USD of the USDY token.

Architecture Feedback, Centralization & Systemic Risks

• The entire architecture is very robust and well implemented, using the Axelar Network as the crosschain message layer removes a lot of burden to bridge assets and increases the security because to execute operation on a Destination Chain, the Axelar Network must validate that a message was sent from the SourceBridge, otherwise, tokens can't be minted in the Destination Chain, thus, if the message was sent from a valid SourceBridge it means that tokens have been already burnt.

• The existing oracle is quite centralized because the price is updated manually, but this is by design because the assets that are tracked varies and right now I believe there is not an existing oracle that tracks the value of those assets.

• There are no major problems in the rUSDY contract, the logic to wrap,unwrap and transfer rUSDY is pretty solid. Scaling up 1:10000 the ratio of USDY:rUSDY facilitates the operations in the rUSDY contract.

Recommendations

• Based on the issues I caught on the codebase I'd recommend the protocol/devs to keep in mind that in most of the blockchains, the tx are not executed as they are sent, which opens up the doors to frontrun some tx that could end up affecting other transactions, this could lead to undesired results such as users gainning more allowance or preventing the burning of their tokens if the protocol has decided to seize their USDY tokens for any reason.

• A think that it might be worth to consider is about how to mitigate edge cases such as what to do when messages fails while they are processed in the Axelar Network. Once a message is sent to the Axelar Network, the user's USDY tokens have already been burnt on the Source Chain, so, if by any reason the message is not posted on the Destination Chain, the user will lost their USDY tokens in the Source Chain because the existing implementation have no means to confirm that the tokens were indeed minted in the Destination Chain

• Yes, the codebase is quite centralized, but that is by design since Ondo is an institutional-grade finance tokenizing short-term US Treasuries and bank demand deposits, I believe the relationship between the protocol and their users will be bounded by a lot of legal paper work, thus, Ondo protocol may not be incentivized to abuse the privileges they have over the code.

• A personal recommendation would be to incentivize the protocol to explore how could they make more decentralize and autonomous their codebase, take a look at existing decentralized solutions that pulls data from the real world into the blockchain (chainlink is currently the leader on this field).

Time Spent on the Audit

• I spent around 5 days on this audit, each day I put in around 7-10 hours of work. In total that would be around 40 - 50 hours.

Time spent:

50 hours