Ondo Finance - ni8mare's results

Lines of code

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

Vulnerability details

Impact

In the rOUSG contract, the burn function calls the burnShares function, which internally calls the _beforeTokenTransfer function. Have a look at this check in the _beforeTokenTransfer function:

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

As the tokens are being burnt, the execution enters the above check as the from address is not 0. The problem lies in the require statement. It checks whether the KYC status of the from address is true. If false, the execution reverts.

This works well for users who have a positive KYC status. But, when the admin wants to burn the tokens from a user whose KYC status was removed, they wouldn't be able to do so. As the _getKYCStatus would output false for a user who was removed from the KYC registry and hence the function reverts.

Note that in the contest readme, the KYC related issue talks about this:

specifically when a user’s KYCRegistry or Sanction status changes in between different actions, leaving them at risk of their funds being locked. If someone gets sanctioned on the Chainalysis Sanctions Oracle or removed from Ondo Finance’s KYC Registry their funds are locked.

This is different from the issue being discussed here. The known issue talks about changing KYC status in between different actions. But, for this issue, KYC status need not change in between actions.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

The above check can be updated to have the following:

if (from != address(0) && to != address(0))

This would allow tokens to be burnt even from a user who does not have a positive KYC status.

Assessed type

Invalid Validation

[L - 1] - The require statements in setMintFee and setRedeemFee are wrong

The require statement in setMintFee is as follows:

require(mintFee < 200, "OUSGInstantManager::setMintFee: Fee too high");

The condition is checking the max fee condition on the state variable mintFee which has already been set. Instead, it should be checking this condition on _mintFee argument in the function. Because of this improper check, it still allows the CONFIGURER_ROLE to set the mint fees to a higher value than 200.

Similarly, redeem fees can be set higher than 200 by the CONFIGURER_ROLE.

Code snippet: https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L557 https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L570

[L - 2] Incorrect check in the require statement of _redeemBUIDL function

  function _redeemBUIDL(uint256 buidlAmountToRedeem) internal {
    require(
      buidl.balanceOf(address(this)) >= minBUIDLRedeemAmount,
      "OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance"
    );

The _redeemBUIDL function, the check buidl.balanceOf(address(this)) >= minBUIDLRedeemAmount should be buidl.balanceOf(address(this)) >= buidlAmountToRedeem, as the amount of tokens being transferred equals buidlAmountToRedeem and this amount can be more than minBUIDLRedeemAmount. Hence, the check should be updated.

[L - 3] Redundant allowance checks

For example, in the redeem function, this allowance check is redundant:

    require(
      ousg.allowance(msg.sender, address(this)) >= ousgAmountIn,
      "OUSGInstantManager::redeem: Insufficient allowance"
    );

This is because in the following transferFrom call in redeem function, this check is internally being called. Hence, the above check is not needed. It increases the gas cost of the function.

[L - 4] Users can prevent their tokens from being burnt by the admin

It was made clear by the sponsor that a KYCed account can hold multiple addresses. So, if they ever notice a transaction by the admin that is trying to burn their tokens, they can send small amounts of their balance to multiple addresses that they own by exploiting this condition in the burn function:

    if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
      revert UnwrapTooSmall();

This check is redundant. It could have been used to prevent zero-value transfers in the following transfer call. But, for that a check to see whether the amount being transferred is above zero would suffice. Hence, the check above can be removed.