Ondo Finance - Shubham's results

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L632 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L560 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L597-L600

Vulnerability details

The function burn is made so the admin can burn rUSDY tokens from any account (this is stated in the comments). However, the admin can’t burn tokens if the account from which they’re trying to burn tokens is blocklisted/sanctioned/not on the allow-list.

Note - The current bug has been reported in the previous audit but still persists in the existing codebase. This does not come under publicly known issues in the current contest scope 'Sanction or KYC related edge cases' given that even though the blocklisted address's funds are locked, the admin has the power to burn the shares. It is also stated that having the BURNER role allows the user to 'burn other holders' rOUSG tokens'. But since the admin is unable to burn those shares, the given bug still persists & is valid.

Proof of Concept

Admin can call burn function to burn rOUSG tokens from any account as intended. This function calls _burnShares().

File: rOUSG.sol

  function burn(
    address _account,
    uint256 _amount
  ) external onlyRole(BURNER_ROLE) {
    uint256 ousgSharesAmount = getSharesByROUSG(_amount);
    if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
      revert UnwrapTooSmall();

    _burnShares(_account, ousgSharesAmount); --> @note

    ...

The _burnShares() calls _beforeTokenTransfer() to check if the account to burn is kyc'd or not.

File: rOUSG.sol

  function _burnShares(
    address _account,
    uint256 _sharesAmount
  ) internal whenNotPaused returns (uint256) {
    require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS");

    _beforeTokenTransfer(_account, address(0), _sharesAmount);  --> @note
  ...

File: rOUSG.sol

  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(_getKYCStatus(msg.sender), "rOUSG: 'sender' address not KYC'd");
    }

    if (from != address(0)) {
      // If not minting
      require(_getKYCStatus(from), "rOUSG: 'from' address not KYC'd");
    }

    if (to != address(0)) {
      // If not burning
      require(_getKYCStatus(to), "rOUSG: 'to' address not KYC'd");
    }
  }

In our case, the from would be the account from which we’re burning tokens, so it’ll enter in the 2nd if: if (from != address(0)). But given that the account is blocked/sanctioned/not on the allow-list, the transaction will revert and the tokens won’t be burned.

Impact

Tokens can't be burned from removed kyc/blocklisted accounts by the admin

Tools Used

Manual Review

Recommended Mitigation Steps

When calling burn(), burn the shares of the contract directly without calling _burnShares(). This way the transaction does not call _beforeTokenTransfer and does not revert if the account the shares should be burned from is removed from the kyc list.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L98-L102 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L554 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L567

Vulnerability details

Users can mint & redeem OUSG and rOUSG against USDC in the OUSGInstantManager contract. During this process, fee is collected from the user which can be same or different for either the minting or redeeming upto a certain limit. The fee can also be 0 if the protocol chooses so.

However both the minting & the redeeming fee is set to 0 at the time of deployment allowing early users to have an upper hand against the rest of the users.

Given that there is no documentation regarding this behaviour in the publicly known issues or intended by the developers & that the loss can be huge, the given issue comes under Medium severity.

Proof of Concept

The fee is set to 0 initially.

File: OUSGInstantManager.sol

  // Fee collected when minting OUSG (in basis points)
  uint256 public mintFee = 0;

  // Fee collected when redeeming OUSG (in basis points)
  uint256 public redeemFee = 0;

The fee can be set by a trusted address with an upper limit as can be seen in the require statements.

File: OUSGInstantManager.sol

  function setMintFee(
    uint256 _mintFee
  ) external override onlyRole(CONFIGURER_ROLE) {
    require(mintFee < 200, "OUSGInstantManager::setMintFee: Fee too high");
    emit MintFeeSet(mintFee, _mintFee);
    mintFee = _mintFee;
  }


  function setRedeemFee(
    uint256 _redeemFee
  ) external override onlyRole(CONFIGURER_ROLE) {
    require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high");
    emit RedeemFeeSet(redeemFee, _redeemFee);
    redeemFee = _redeemFee;
  }

However inside the constructor both the fees are not set & will be 0 until the set functions are called by the configurer. This would allow early users to mint/redeem tokens without paying anything to the protocol which would be a loss to the protocol.

Loss to the protocol is proportional to the amount of tokens being minted/redeemed.

In the event if an attacker carried out a flashloan with a huge amount of usdc to mint & redeem tokens, this would result in a huge loss to the protocol because the fee is set to 0 at this point.

Impact

Loss of funds to the protocol because no fee is assigned for minting & redeeming.

Tools Used

Manual Review

Recommended Mitigation Steps

Set the minting & redeeming fee inside the constructor.

Assessed type

Error