Lack of slippage control in ROUSG::burn() & ROUSG::unwrap()

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L624-L640

Vulnerability details

Summary

The ROUSG.burn() function allows admin to burn rOUSG tokens from any _account thereby getting OUSG in return. The ROUSG.unwrap() on the other hand is called by users to unwrap their rOUSG tokens.

However, these functions do not have any type of slippage control since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

Also the users will have no defense against price manipulation attacks, if they were to be found after the protocol's deployment.

Proof of Concept

Instance 1: ROUSG.burn()

• https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L624-L640

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

    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );
    emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
    emit TransferShares(_account, address(0), ousgSharesAmount);
  }

The ousgSharesAmount to be burned is determined by the getSharesByROUSG() function of the same contract:

• https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L363-L368

  function getSharesByROUSG(
    uint256 _rOUSGAmount
  ) public view returns (uint256) {
    return
      (_rOUSGAmount * 1e18 * OUSG_TO_ROUSG_SHARES_MULTIPLIER) / getOUSGPrice();
  }

As can be observed, this function uses oracle interaction through the division by getOUSGPrice(). The getOUSGPrice() function queries the external oracle for the asset price:

• https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L378-L380

  function getOUSGPrice() public view returns (uint256 price) {
    (price, ) = oracle.getPriceData();
  }

To determine the amount of OUSG to be received after share burning, the ousgSharesAmount calculated above is divided by OUSG_TO_ROUSG_SHARES_MULTIPLIER:

• https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L634-L637

    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );

This means that the user has no way to predict how many OUSG they will get back at the moment of burning shares, as the price could be updated while the request is in the mempool.

Instance 2: ROUSG.unwrap()

• https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L431-L443

  function unwrap(uint256 _rOUSGAmount) external whenNotPaused {
    require(_rOUSGAmount > 0, "rOUSG: can't unwrap zero rOUSG tokens");
    uint256 ousgSharesAmount = getSharesByROUSG(_rOUSGAmount);
    if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
      revert UnwrapTooSmall();
    _burnShares(msg.sender, ousgSharesAmount);
    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );
    emit Transfer(msg.sender, address(0), _rOUSGAmount);
    emit TransferShares(msg.sender, address(0), ousgSharesAmount);
  }

ousgSharesAmount is detamined by calling getSharesByROUSG():

  function getSharesByROUSG(
    uint256 _rOUSGAmount
  ) public view returns (uint256) {
    return
      (_rOUSGAmount * 1e18 * OUSG_TO_ROUSG_SHARES_MULTIPLIER) / getOUSGPrice();
  }

which also uses getOUSGPrice() in division as above. This invokes the external oracle to get the asset price:

  function getOUSGPrice() public view returns (uint256 price) {
    (price, ) = oracle.getPriceData();
  }

And finally to determine the amount of OUSG to be received after unwrapping, the ousgSharesAmount calculated above is divided by OUSG_TO_ROUSG_SHARES_MULTIPLIER:

    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );

Similarly, the user has no way to predict how many OUSG they will get back at the moment of unwrapping, as the price could be updated while the request is in the mempool.

Tools Used

Manual Review

Recommended Mitigation Steps

Add additional parameter that could be added in these functions, to decide the minimum amount of tokens to be received, with a relative check after burning/unwrapping.

Assessed type

Oracle

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #156

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as primary issue

[c4-judge]

3docSec marked the issue as selected for report

[Brivan-26]

I'm quoting from the warden's description:

However, these functions do not have any type of slippage control since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

I believe this is wrong, the amount to receive will never vary indefinitely as mentioned, because Oracle doesn't allow %1 price change when updating the price. So the user may still view the current OUSG price and can still determine the outcome of the transaction.

This means that the user has no way to predict how many OUSG they will get back at the moment of burning shares, as the price could be updated while the request is in the mempool.

This is very unlikely to happen, the price is updated once every 23 hours at best scenario. So an unwrap/burn transaction encounter the oracle::setPrice in the mempool is very unlikely. Even if such rare cases happen, the price won't change too much as the oracle doesn't allow a change slippage of %1

[3docSec]

☝️ this is a very good point.

In summary:

• only admins can change the price
• admins can't make updates before 23 hours passed since last update
• even after waiting 23 hours, admins are not allowed to make a > 1% price change since last update

All of the above give factual proof that slippage is negligible in this context. 💯 @Brivan-26

[3docSec]

Update, it's actually 2% in RWAOracleExternalComparisonCheck but the argument stands.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[c4-judge]

3docSec marked the issue as not selected for report

[0xbtk]

Hey @3docSec, 2% may seem small, but it's not negligible. A significant number of users are likely to utilize both redeemRebasingOUSG and mintRebasingOUSG, with a minimum requirement of 50k for redemption and 100k for minting. Hence, a 2% loss translates to 1k during redemption and 2k during minting, posing a non-trivial risk of asset loss, warranting a medium severity i believe.