search

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

5 stars 6 forks source link

Partial Pausing of either mint or redeem can lead to arbitrage opportunities for users #317

Closed c4-bot-5 closed 3 months ago

c4-bot-5 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L780 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L768

Vulnerability details

Minting & Redeeming inside the OUSGInstantManager contract can be paused by the admin having the PAUSER_ROLE role. However both these functions are not paused at once but can be done individually. Only the admin having the DEFAULT_ADMIN_ROLE can unpause.

Note - The publicly known issues does address the arbitrage risks by front running but nowhere in the scope it is mentioned issues related with partial Pausing functionality.

Proof of Concept

pauseMint() & pauseRedeem() are called by the admin to pause minting & redeeming inside the instant manager contract.

File: ousgInstantManager.sol

  function pauseMint() external onlyRole(PAUSER_ROLE) {
    mintPaused = true;
    emit MintPaused();
  }
  ...
  function pauseRedeem() external onlyRole(PAUSER_ROLE) {
    redeemPaused = true;
    emit RedeemPaused();
  }

Scenario 1:

Scenario 2:

Impact

Partial pausing causes arbitrage opportunities to the users

Tools Used

Manual Review

Recommended Mitigation Steps

Make a single pause function that pauses both minting & redeeming at once by the pauser.

Assessed type

Timing

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as insufficient quality report

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as primary issue

0xRobocop commented 3 months ago

Invalid

3docSec commented 3 months ago

Price of ousg starts is a key step in the finding's tokenomics reasoning, and is factually incorrect, because the OUSG price is manually updated by the protocol admins.

c4-judge commented 3 months ago

3docSec marked the issue as unsatisfactory: Invalid

Shubh0412 commented 3 months ago

Hello @3docSec

I agree that the price can be manually updated every 23 hours with a deviation of 1% as can be seen in RWAOracleRateCheck contract. 

File: RWAOracleRateCheck.sol

      function setPrice(int256 newPrice) external onlyRole(SETTER_ROLE) {
        if (newPrice <= 0) {
          revert InvalidPrice();
        }
        if (block.timestamp - priceTimestamp < MIN_PRICE_UPDATE_WINDOW) {
          revert PriceUpdateWindowViolation();
        }
        if (_getPriceChangeBps(rwaPrice, newPrice) > MAX_CHANGE_DIFF_BPS) {
          revert DeltaDifferenceConstraintViolation();
        }
        ...

Expanding 'Scenario 2' of the original submission -

Expanding on 'Scenario 1', when the price decreases,

The original issue is the fact that partially pausing only minting or redeeming & not both will lead to arbitrage opportunities.

Given the fact that the PAUSER_ROLE can be granted to a different trusted person & 'unpause' is called by the DEFAULT_ADMIN_ROLE & 'setPrice()' by the SETTER_ROLE, the likelihood of them being out of sync, the unsure duration of how long the mint/redeem functionality will be paused & the price being updated only after 23 hours, the likelihood of the above scenarios is high with high arbitrage opportunities.

3docSec commented 3 months ago

Some users decide to redeem to minimize their losses but the function reverts. Meanwhile minting is still active

I believe this is the intended behavior and the reason why the team decided to split the two. I don't see this as a bug but a feature - I imagine the team will want to pause redeeming and minting at the same time for the reasons you mentioned, but wants to be open to handle situations we/they didn't think of yet, with more granular control. I don't see anything wrong with that.

Shubh0412 commented 3 months ago

Hello again @3docSec

I imagine the team will want to pause redeeming and minting at the same time for the reasons you mentioned

Given the current codebase in scope, pausing mint & redeem at different times does leads to the scenarios as described above with valid opportunities to the users for arbitrage. As of now, pausing does occur differently without any reasoning by the admins as to why the protocol does so. Even if they did, front-running the pause functionality would still be an issue in this case with the same impacts as described above.

but wants to be open to handle situations we/they didn't think of yet, with more granular control.

Nowhere under the publicly known issues or inside the docs of the protocol are the effects or use cases related to pausing mentioned.

How the protocol decides to handle the unknown situations in the future might be appropriate for a different audit/contest but under the scope of the current audit, the above issue does define a valid attack path & its impact.

Would request the judge to take another look & consider this to be a Medium severity issue.

3docSec commented 3 months ago

I see your point, but the decision is final; this finding doesn't meet the HM bar, as admins are assumed to know what they are doing when pausing/unpausing their contracts.

Shubh0412 commented 3 months ago

@3docSec

According to the C4 guidelines

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Here 'leak value' would be price change of ousg which cannot be changed for a period of 23 hours. 'attack path' would be the scenarios explained in the report & 'external requirements' would be pausing by the admins.

Given that this submission focuses on pausing functionality but also points out the root cause mentioned in issue #278 which is excessive minting incase of price change, if not as a separate finding would you consider giving it credit under issue 278?

I rest my case. Thank you taking out the time to converse with me on this finding & apologizes for the trouble caused.