InstantManager attempts to redeem BUIDL even if it has enough USDC in the contract

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Redemptions of amounts greater than minBUIDLRedeemAmount will revert even if there are enough USDC tokens in the contract to perform a payoff.

Proof of Concept

During the redemption, a situation may occur when there are not enough BUIDL tokens in the contract to perform _redeemBUIDL, but the USDC balance is greater than usdcAmountToredeem. Because we check usdcAmountToRedeem >= minBUIDLRedeemAmount before usdcAmountToRedeem > usdcBalance, the contract will attempt to redeem BUIDL tokens every time and fail.

>>  if (usdcAmountToRedeem >= minBUIDLRedeemAmount) {
      // amount of USDC needed is over minBUIDLRedeemAmount, do a BUIDL redemption
      // to cover the full amount
      _redeemBUIDL(usdcAmountToRedeem);
    } else if (usdcAmountToRedeem > usdcBalance) {
      // There isn't enough USDC held by this contract to cover the redemption,
      // so we perform a BUIDL redemption of BUIDL's minimum required amount.
      // The remaining amount of USDC will be held in the contract for future redemptions.
      _redeemBUIDL(minBUIDLRedeemAmount);
      emit MinimumBUIDLRedemption(
        msg.sender,
        minBUIDLRedeemAmount,
        usdcBalance + minBUIDLRedeemAmount - usdcAmountToRedeem
      );
    } else {
      // We have enough USDC sitting in this contract already, so use it
      // to cover the redemption and fees without redeeming more BUIDL.
      emit BUIDLRedemptionSkipped(
        msg.sender,
        usdcAmountToRedeem,
        usdcBalance - usdcAmountToRedeem
      );
    } 

A malicious user can exploit this vulnerability and make the contract redeem all it's BUIDL tokens with multiple mint/redeem runs (possibly involving flashloans). As a result, all redemptions with amounts greater than minBUIDLRedeemAmount will fail.

Tools Used

Manual review

Recommended Mitigation Steps

Consider refactoring the condition statement as follows

    if (usdcAmountToRedeem <= usdcBalance) {
      // We have enough USDC sitting in this contract already, so use it
      // to cover the redemption and fees without redeeming more BUIDL.
      emit BUIDLRedemptionSkipped(
        msg.sender,
        usdcAmountToRedeem,
        usdcBalance - usdcAmountToRedeem
      );
    } else if (usdcAmountToRedeem > minBUIDLRedeemAmount) {
      // amount of USDC needed is over minBUIDLRedeemAmount, do a BUIDL redemption
      // to cover the full amount
      _redeemBUIDL(usdcAmountToRedeem);
    } else {
      // There isn't enough USDC held by this contract to cover the redemption,
      // so we perform a BUIDL redemption of BUIDL's minimum required amount.
      // The remaining amount of USDC will be held in the contract for future redemptions.
      _redeemBUIDL(minBUIDLRedeemAmount);
      emit MinimumBUIDLRedemption(
        msg.sender,
        minBUIDLRedeemAmount,
        usdcBalance + minBUIDLRedeemAmount - usdcAmountToRedeem
      );
    }

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[cameronclifton]

I think there may be something here - can we provide some examples with real numbers?

[c4-sponsor]

cameronclifton (sponsor) acknowledged

[3docSec]

The finding is valid, and the recommended mitigation is very sound, so the sponsor is warmly recommended to consider it.

However, for fairness, we'll need to close this issue because the README noted this scenario as a publicly known issue.

We are aware that if all BUIDL has been redeemed from our contract, we would not be able to provide instant redemptions for OUSG or rOUSG. We are also aware that there may be some USDC leftover in the contract that wouldn't be redeemable in this scenario.

Honorable mention to #235 that, too, highlights how the redeem logic is stricter than it could.

[c4-judge]

3docSec marked the issue as unsatisfactory:
Out of scope

[c4-sponsor]

cameronclifton (sponsor) confirmed

[cameronclifton]

We will be addressing this mitigation, I think this is a good finding.

[k4zanmalay]

@3docSec

We are aware that if all BUIDL has been redeemed from our contract, we would not be able to provide instant redemptions for OUSG or rOUSG. We are also aware that there may be some USDC leftover in the contract that wouldn't be redeemable in this scenario.

I'm afraid this statement is too broad, as it encompasses scenarios in which there may not be enough BUIDL tokens to cover redemptions due to normal activity, as well as legit attack cases where BUIDL was removed from the contract by a malicious user. Can we please ask sponsors to clarify what did they mean in the "Known Issues"? It seems unlikely that they would knowingly leave such a vulnerability that can be exploited, like it was shown in this report and it's dupes. Thank you.

[3docSec]

Hi @k4zanmalay we can absolutely have the sponsor clarify.

However, clarification should've been done before the contest ended, and made public to all wardens to be considered in judging: we may very well have had other wardens notice this finding, and not report it because they thought it fell under this known issue; considering it valid depending on the sponsor clarifying their intention after the contest would be unfair.

[k4zanmalay]

Hi @3docSec I believe it should be treated as escalation of already known issue, like it's done with findings based on bot reports
https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#recommendation-standardize-acceptance-of-reports-based-on-automated-findings
Based on the sponsor's comment, this report and its duplicates provided new valuable information on how this issue could be exploited by a malicious actor.

[3docSec]

A bot finding typically has a root cause, an impact, and a mitigation that may make it inaccurate. A "known issue" from the sponsor doesn't, so what you say sounds a bit forced.

However, I definitely see a margin for improving our judging guidelines, especially on how known issues can be interpreted and to what extent are they open to escalations, like you said, like bot reports are. If you open an issue here (just make sure there isn't one already!) I'll be happy to support it.