Inconsistent Balance Verification in _redeemBUIDL Function

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L426-L440
https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L458-L469

Vulnerability details

Impact

The _redeemBUIDL function allows for the redemption of BUIDL tokens for USDC tokens. But based on the code line 426 - line 440,

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

We can take in input under 2 different conditions into the redeem function to perform the necessary logic. The impact of the finding is that there might be a potential vulnerability or issue related to the accurate check of the buidl balance during the redemption process when we are redeeming more than the minimum, the contract has the monet when it's USDC Amount are combine but have less total buidl to cover, eventhough we have enough more buidl and usdc the transaction will fail.

Proof of Concept

Scenario 1

1. Alice wants to redeem 265,000 worth of usdc token, inputed the corresponding amount of OUSG token.

2. The token is converted to 264,950 after conversion and fee removal. Amount to redeem is still 265,000.

3. The code calls

 if (usdcAmountToRedeem >= minBUIDLRedeemAmount) {
      // amount of USDC needed is over minBUIDLRedeemAmount, do a BUIDL redemption
      // to cover the full amount
      _redeemBUIDL(usdcAmountToRedeem);
    }

4. function _redeemBUIDL is called

function _redeemBUIDL(uint256 buidlAmountToRedeem) internal {
  require(
    buidl.balanceOf(address(this)) >= minBUIDLRedeemAmount,
    "OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance"
  );
  uint256 usdcBalanceBefore = usdc.balanceOf(address(this));
  buidl.approve(address(buidlRedeemer), buidlAmountToRedeem);
  buidlRedeemer.redeem(buidlAmountToRedeem)
  require(
    usdc.balanceOf(address(this)) == usdcBalanceBefore + buidlAmountToRedeem,
    "OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1"
  );
}

5. check 1

- require(
      buidl.balanceOf(address(this)) >= minBUIDLRedeemAmount,
      "OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance"
    );
 

this contract checks
the buidl balance of this address.
balance=255,000,
minBUIDLRedeemAmount = 250,000.
uint256 usdcBalanceBefore = 0.

6. After other checks

require(
    usdc.balanceOf(address(this)) == usdcBalanceBefore + buidlAmountToRedeem,
    "OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1"
  ); 

255,000== 0+265,000.
will throw a error OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1.

A user become curious checks the ratio of buidl to usdc on the website an it shows 1:1 tries again but the transaction fails again.
The contract fails to assert if it has enough to proceed with the transaction it only checks if we have enough to cover minBUIDLRedeemAmount.

Second scenario

We have enough USDC and Buidl to complete the transaction but the transaction will still fail

If the contract has enough enough Usdc or buidl+Usdc, and buidl balance of the contract is more than the minimum to redeem but the transaction still fails

1. Alice wants to redeem 265,000 worth of usdc token, inputed the corresponding amount of OUSG token.

2. The token is converted to 264,950 after conversion and fee removal. Amount to redeem is still 265,000.

3. The check below

 if (usdcAmountToRedeem >= minBUIDLRedeemAmount) {
      // amount of USDC needed is over minBUIDLRedeemAmount, do a BUIDL redemption
      // to cover the full amount
      _redeemBUIDL(usdcAmountToRedeem);
    }  

is true but because the usdcAmountToRedeem is above our balance we can't cover for it .

4 . the transaction will (proceed to redeem the user's buidl instead when we don't have enough buidl balance the check checks minimum not taking into account the dynamic nature of user's and the amount they want to withdraw) even though approve is given to the buidl to redeem the transaction will fail due to insufficient balance. thus the check that should have caught this will fail.

buidl.approve(address(buidlRedeemer), buidlAmountToRedeem);
    buidlRedeemer.redeem(buidlAmountToRedeem); 

failed check

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

from this scenario the user is not paid even though we have enough usdc because the necessary check fails even though we have enough to cover the payment.

Tools Used

Manual Code analysis.

Recommended Mitigation Steps

To address the potential vulnerability in the _redeemBUIDL function, consider the following steps:

Accurate Balance Calculation and Additional Checks Implement additional checks to verify the approval and redemption processes.
Ensure the accurate calculation so that a user gets paid once we have enough balance or at less some usdc balance with buidl above minimum as long as they can cover for the about to be redeemed amount.

uint256 Buidlbal = buidl.balanceOf(address(this));
uint256 usdcBalanceBefore = usdc.balanceOf(address(this));
uint256 TotalBalance = Buidlbal + usdcBalanceBefore;

if (TotalBalance >= buidlAmountToRedeem && Buidlbal >= buidlAmountToRedeem && Buidlbal >= minBUIDLRedeemAmount) {
    buidl.approve(address(buidlRedeemer), buidlAmountToRedeem);
    buidlRedeemer.redeem(buidlAmountToRedeem);
require(
      usdc.balanceOf(address(this)) == usdcBalanceBefore + buidlAmountToRedeem,
      "OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1"
    );
} 
else if (TotalBalance >= buidlAmountToRedeem && Buidlbal < buidlAmountToRedeem && Buidlbal >= minBUIDLRedeemAmount) {
    buidl.approve(address(buidlRedeemer), Buidlbal);
    buidlRedeemer.redeem(Buidlbal);
require(
      usdc.balanceOf(address(this)) == usdcBalanceBefore + Buidlbal,
      "OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1"
Buidlbal+= buidlAmountToRedeem - Buidlbal;// increment balance of contract with the difference 
    );

}else {
    revert("InadequateUSDCbalance");
}

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #267

[c4-pre-sort]

0xRobocop marked the issue as not a duplicate

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #179

[c4-judge]

3docSec marked the issue as unsatisfactory:
Insufficient proof

[lanrebayode]

Thank you for judging @3docSec,

I believe this issue might be on course, This issue is completely different from the main issue #179, it outlines a possible denial of service even if the protocol has enough funds.
The judge can decide to have a second look, it looks similar to #142 and an issue that was acknowledged by sponsor #109. EVEN if the above issues mitigations are followed the issue stated here will still be present. This finding is not THE SAME as #109 and #142 and is completely different from the Readme. According to the warden, there is a possible denial of service even if we have enough buidl and also have redeemable usdc but redemption will still fail. Because the protocol is checking build>=minimum(250k), they should be checking buidl balance>=buidl to redeem (which can be above 250k) since it is already a fact that we cannot redeem less than 250k.
The above MITIGATION allows for the possibility to pay users when we have enough considering the different possibilities.

According to the doc

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.

From the DOC statement nothing points to the protocol not having enough buidl to be redeemed in this findings because we have more than the minimum and nothing points to an unredeemable usdc and yet the USER will be denied.

[3docSec]

Scenario 1: the revert will happen only if the redeemer sends back fewer USDC tokens than BUIDL sent, which is precisely #175
Scenario 2: this looks really like #109, so I'll move this finding there.

However:

We are aware that if all BUIDL has been redeemed from our contract [...] there may be some USDC leftover in the contract that wouldn't be redeemable in this scenario.

is very much like:

even though approve is given to the buidl to redeem the transaction will fail due to insufficient balance [...] the user is not paid even though we have enough usdc

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by 3docSec

[c4-judge]

3docSec marked the issue as duplicate of #109

[c4-judge]

3docSec marked the issue as unsatisfactory:
Out of scope

[lanrebayode]

Thanks once again for judging @3docSec,

i believe this issue is a duplicate of #306, both speak on the redemption function unlike #109 and their mitigation is the same.

[lanrebayode]

@3docSec still awaiting your response.

like i have explained initially, a complete review of this shows that it is a duplicate of #306 . same impact,function and mitigation.

To address this issue, the OUSG Instant Redemption Manager contract should implement a mechanism to ensure that redemption requests do not exceed the available BUIDL balance held by the manager contract. This can be achieved by incorporating proper checks and balances in the redemption process, such as verifying the BUIDL balance before processing redemption requests and adjusting the redemption amount accordingly. Additionally, consider an redeem implementation that concatenate the balance of remaining USDC amount with the BUIDL redeemed balance if the corresponding USDC amount or redeem amount of OUSG is more than minBUIDLRedeemAmount.

mitigation from #306 a review shows total similarity. and from your initial comment on both issue it sounds like but it is not, has confirmed by the sponsor in 306.

[3docSec]

Hi @lanrebayode I reviewed attentively your claim, and no, scenario 2 is not #306.

The finding in 306 points out a scenario where the contract's BUILD balance is higher than minBUIDLRedeemAmount, so this specific part of your PoC:

failed check

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

makes it unequivocally clear that the root cause pointed out here is not the same as #306, and that this finding falls in the same out of scope criteria as #109 :

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.

[lanrebayode]

@3docSec
Based on the scenario you mention line 2 of scenario 2 as stated by the warden

If the contract has enough enough Usdc or buidl+Usdc, and buidl balance of the contract is more than the minimum to redeem but the transaction still fails
which is clearly the same as #306

also look at th wardens mitigation

else if (TotalBalance >= buidlAmountToRedeem && Buidlbal < buidlAmountToRedeem && Buidlbal >= minBUIDLRedeemAmount) {
buidl.approve(address(buidlRedeemer), Buidlbal);
buidlRedeemer.redeem(Buidlbal);
require(
usdc.balanceOf(address(this)) == usdcBalanceBefore + Buidlbal,
"OUSGInstantManager::_redeemBUIDL: BUIDL:USDC not 1:1" );
Buidlbal+= buidlAmountToRedeem - Buidlbal;// increment balance of contract with the difference

this clearly covers all the stated conditions with a well thought out mitigation and is completely the same has #306.

[3docSec]

Hi, you are right, the second scenario explicitly mentions that the contract has more than the minimum, the sum of usdc + buidl balance is enough, and the transaction fails. The mitigation also makes sense in context with #306.

I am duping this to #306, but in your further contributions you really should improve the clarity of your reports:

• if you have two root causes (scenario 1 and scenario 2), make 2 separate findings
• proofread your sentences, some are really hard to follow
• verify the accuracy of your technical statements, or even better, include a coded PoC.

Why the last point? The below statement is NOT where your scenario would fail because of the assumption you made and buidl balance of the contract is more than the minimum to redeem, and this incorrect assumption was a pretty convincing argument for not giving credit.

failed check

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

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec marked the issue as duplicate of #306

[c4-judge]

3docSec marked the issue as satisfactory