code-423n4 / 2024-03-ondo-finance-findings

5 stars 6 forks source link

Stuck `OUSG` tokens at `rOUSG` contract due to rounding down operations #330

Closed c4-bot-4 closed 3 months ago

c4-bot-4 commented 3 months ago

Lines of code

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

Vulnerability details

Users can wrap OUSG tokens into rOUSG tokens by calling rOUSG::wrap and unwrap them by calling rOUSG::unwrap. The function unwrap() will convert the amount given on the argument, i.e. rOUSG tokens, to shares in order to burn those shares and transfer the OUSG tokens to the user. The calculations are the following:

$$ \begin{aligned} \text{ousgSharesAmount} &= rOUSGAmount \times \frac{sharesMultiplier \times 1e18}{oUSGPrice} \\ \text{ousgTokensAmount} &= \frac{ousgSharesAmount}{sharesMultiplier} \end{aligned} $$

The value of ousgTokensAmount will be the amount of OUSG tokens that the contract will send the user that is unwrapping the rOUSG tokens.

Because solidity rounds down by default, the value of ousgTokensAmount will be rounded down. This is because the value of oUSGPrice will always be higher than 1e18, in fact, will always be higher than 105e18, as per the ousgInstantManager contract indicates.

Therefore, after a user wraps and unwraps some tokens, some tokens will be left at the rOUSG contract. This amount may be tiny in a single unwrap operation, but it will become increasingly large over time. This will lead to a significant amount of oUSG tokens being stuck at the rOUSG contract as time passes.

Impact

Over time, OUSG tokens will get stuck on the rOUSG contract.

Proof of Concept

The unwrap function, the two indicated lines of code is where the rounding down happens:

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/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);
  }

And the getSharesByROUSG function is always rounding down because the OUSG price will always be bigger than 1e18. In fact, it will always be higher than 105e18, as per the ousgInstantManager contract.

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to create a special function for the protocol team to rescue the stuck OUSG tokens from the rOUSG contract.

Assessed type

Math

0xRobocop commented 3 months ago

OOS

Rebasing Precision Issue - rOUSG is a rebasing tokens and some precision can be lost when wrapping and unwrapping. We expect this to only result in extremely small precision discrepancies. However, if one can find a realistic usage of the contracts that results in a non-trivial amount of imprecision, it would be valid.

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as insufficient quality report

c4-judge commented 3 months ago

3docSec marked the issue as unsatisfactory: Out of scope