Inadequate Handling of BUIDL Redemption Limit in OUSG Instant Manager

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The OUSG Instant Redemption Manager contract contains an oversight in its redeem function, specifically in the handling of BUIDL redemption limits. This oversight can potentially lead to failed redemption attempts when the redemption balance exceeds the BUIDL balance held by the manager contract while it has a right amount if its concatenated with USDC amount left by another redemption process. The impact of this issue is significant as it affects the usability of the redemption feature and can result in user frustration and loss of trust in the system.

Proof of Concept

The following Proof of Concept (POC) demonstrates the issue, use it as part of forge-tests/ousg/OUSGInstantManager/redeem.t.sol file:

Run using this command

npm run test-forge -- --match-test test_POC_reddem_fail_when_alice_redeemtion_balance_is_over_manager_BUIDL_balance

  function test_POC_reddem_fail_when_alice_redeemtion_balance_is_over_manager_BUIDL_balance()
    public
    setupSecuritize(500_000e6, 500_000e6)
  {    
    uint256 aliceOUSGRedeemAmount = 1667e18; 
    uint256 aliceUSDCAmount = 250_100e6;

    uint256 bobOUSGRedeemAmount = 1666e18;
    uint256 bobUSDCAmount = 249_900e6;

    // Mint OUSG tokens for Alice and Bob
    vm.prank(address(ousgInstantManager));
    ousg.mint(alice, aliceOUSGRedeemAmount);

    vm.prank(address(ousgInstantManager));
    ousg.mint(bob, bobOUSGRedeemAmount);

    // Bob redeems OUSG tokens successfully
    vm.startPrank(bob);
    ousg.approve(address(ousgInstantManager), (bobOUSGRedeemAmount));
    ousgInstantManager.redeem(bobOUSGRedeemAmount);
    vm.stopPrank();

    // Alice attempts to redeem OUSG tokens, but the redemption fails due to insufficient BUIDL balance
    vm.startPrank(alice);
    ousg.approve(address(ousgInstantManager), (aliceOUSGRedeemAmount));
    vm.expectRevert('Not enough tokens');
    ousgInstantManager.redeem(aliceOUSGRedeemAmount);
    vm.stopPrank();

    assertEq(USDC.balanceOf(bob), bobUSDCAmount);

    assertEq(
      BUIDL.balanceOf(address(ousgInstantManager)) + USDC.balanceOf(address(ousgInstantManager)),
      aliceUSDCAmount
    );

    // However if Alice try to reddem an amount that will be in usdc amount <= minBUIDLRedeemAmount in ousgInstantManager it will success
    vm.startPrank(alice);
    ousgInstantManager.redeem(aliceOUSGRedeemAmount - 10e18);
    vm.stopPrank();

    // Tokens remaining in Alice's balance after successful redemption
    assertEq(ousg.balanceOf(alice), 10e18);

  }

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

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.

Assessed type

Error

[0xRobocop]

From the PoC:

assertEq(
      BUIDL.balanceOf(address(ousgInstantManager)) + USDC.balanceOf(address(ousgInstantManager)),
      aliceUSDCAmount
 );

Same problem as #235. Even if the current BUIDL balance + USDC balance of the contract is enough to cover the redeem, the contract cannot redeem BUIDL tokens if it does not have a minimum of 250k.

There is a BUIDL redemption minimum requirement that can assumed to be 250,000 BUIDL

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #235

[c4-judge]

3docSec marked the issue as unsatisfactory: Out of scope

[khalidfsh]

Hi,

I would like to respectfully address the assessment provided for this issue.

Upon careful analysis and consideration, I believe there are aspects of this issue that distinguish it from issue #235, indicating that it is not a duplicate nor out of scope. Allow me to provide a detailed breakdown:

Issue Distinctions:

1. In the PoC of this issue, the Instant Manager contract has the minimum requirement for BUIDL redemption when Alice fails to redeem his share amount. With all respect, pre-sort evident is not true! instant Manager balance at the failed redeem transaction is 250_000e6 BUIDL. This line from the assertion BUIDL.balanceOf(address(ousgInstantManager)) + USDC.balanceOf(address(ousgInstantManager)) represents Alice's share after Bob's redemption, which is equivalent to 250_000e6 + 100e6.

2. In the primary issue it points the fail of the transaction in this require statement:

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

   while in this issue, when Alice try to redeem his OUSG token the revert expected in the PoC specifically by an error that coming from BUIDL redeemer contract (Error:'Not enough tokens'):

   vm.expectRevert('Not enough tokens');
   ousgInstantManager.redeem(aliceOUSGRedeemAmount);

Clarification on Flow of Tokens From PoC:

This flow of tokens elucidates the sequence of events and the impact on user balances during redemption attempts from the PoC.

1. Starting with these balances:

   Tokens	Instant Manager Contract Balances	Alice's Balance	Bob's Balance
   OUSG	0	1,667	1,666
   BUIDL	500,000	0	0
   USDC	0	0	0

2. After Bob redeem his tokens:

   Tokens	Instant Manager Contract Balances	Alice's Balance	Bob's Balance
   OUSG	0	1,667	0
   BUIDL	250,000	0	0
   USDC	100	0	249,900

3. Now when Alice try to redeem his 1667e18 OUSG tokens will fail since the Instant Manager contract try to redeem BUIDL above its balance (250_100e6 BUIDL). However, Alice can only redeem an equivalent amount of OUSG less than or equal to the minimum requirement for BUIDL redemption.

In this PoC Alice only lost 100e6 USDC as it can not be redeemed, but it can be bigger as long as it is less than the minimum redemption requirement.

---

Given the distinctions outlined above and the implications for system functionality and user experience, I respectfully request a reconsideration of the categorization of this issue. It addresses a separate aspect of the contract's behavior and presents a valid concern that warrants attention.

Thank you for your time and consideration.

[3docSec]

Hi,

I've done some debugging on your PoC and I thank you for having included a runnable PoC because it helped me understand how it's indeed different than #285.

The problem you are outlining is that OUSGInstantManager unnecessarily makes the full-amount _redeemBUIDL(usdcAmountToRedeem); swap, because it has sufficient USDC balance to cover the request with a smaller _redeemBUIDL(buidl.balanceOf(address(this))) swap (given that the balance is at least equal to minBUIDLRedeemAmount + using its USDC balance.

I see this problem therefore closer to #109, but not quite the same, because the #109 fix won't fix this issue either.

I also agree that this one does not entirely fit the out-of-scope:

We are aware that if all BUIDL has been redeemed from our contract, we will 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.

because the balance of BUIDL being at least equal to minBUIDLRedeemAmount means that we are not in the if all BUIDL has been redeemed from our contract condition.

[c4-judge]

3docSec marked the issue as not a duplicate

[3docSec]

Hi @cameronclifton we'd appreciate having a sponsor review on this one... could you please share your opinion? 🙏

[cameronclifton]

This was known and supposed to be covered in: `We are aware that if all BUIDL has been redeemed from our contract, we will 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 guess the wording we chose for the above doesn't technically cover this, but it was very much intended to be.

Our operational processes will ensure that there is sufficient BUIDL in our contract at all times relative to our global redemption limits, and do not expect this edge case to be hit.

[3docSec]

I see the point @cameronclifton and I agree with you that the intention is reasonable, but this issue does not clearly fall into the out-of-scope that was set in the README.

It may look like nitpicking, but we want to be fair with the warden's effort and if the finding is valid as per the contest rules, it deserves a reward.

This doesn't mean you will have to fix it, you still have the option to acknowledge it if as you say the processes outside the contest scope are enough to mitigate it.

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[c4-judge]

3docSec marked the issue as primary issue

[Brivan-26]

Hi @3docSec sorry for dropping the comment at this moment, but I just saw this issue is upgraded. There are some contradictions between the warden's description and the contest README phrases

Quoting from the warden's description:

The OUSG Instant Redemption Manager contract contains an oversight in its redeem function, specifically in the handling of BUIDL redemption limits. This oversight can potentially lead to failed redemption attempts when the redemption balance exceeds the BUIDL balance held by the manager contract while it has a right amount if its concatenated with USDC amount left by another redemption process..

This is wrong if we compare what was mentioned in the contest README, Quoting from contest README:

USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function.

It is mentioned that USDC left in the ousgManager contract will be used only to serve redemptions less than the minimumBUIDLRedemption

Also, the warden has referenced this code snippet which is a wrong assumption that the contract may not have enough BUIDL balance. Quoting from contest README:

Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions.

[radeveth]

Hi @3docSec sorry for dropping the comment at this moment, but I just saw this issue is upgraded. There are some contradictions between the warden's description and the contest README phrases

Quoting from the warden's description:

The OUSG Instant Redemption Manager contract contains an oversight in its redeem function, specifically in the handling of BUIDL redemption limits. This oversight can potentially lead to failed redemption attempts when the redemption balance exceeds the BUIDL balance held by the manager contract while it has a right amount if its concatenated with USDC amount left by another redemption process..

This is wrong if we compare what was mentioned in the contest README, Quoting from contest README:

USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function.

It is mentioned that USDC left in the ousgManager contract will be used only to serve redemptions less than the minimumBUIDLRedemption

Also, the warden has referenced this code snippet which is a wrong assumption that the contract may not have enough BUIDL balance. Quoting from contest README:

Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions.

Good Point, @Brivan-26!

[lanrebayode]

Apologies for commenting at this point but there is no contradiction here,

USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function.

"may be utilized" not "will be utilised only for this". May is an if statement and for every if there is an else. Hope this clears the air. Thank you, once again apologies for commenting.

[khalidfsh]

Hi @Brivan-26 Thank you for sharing your thought! You are quoting from the impact of the issue, there is no description in this report. I encourage you to read the PoC for more in-depth details of the issue.

If you see the PoC section, even if the instant manager has enough BUIDL balance to cover all redemptions that can be done, it will fail at some point if this edge case occurs (where the USDC balance leftover from another redemption is part of the issue but it is not the issue itself). Also, here is the full quoting of the known issue you are referring to clearer to be subjected here:

We are aware that USDC may be leftover and held within OUSGInstantManager when a user performs a redemption that is more than the minimumRedemption amount and less than the minimumBUIDLRedemption amount. We are also aware that this may cause temporary "cash drag" for the overall OUSG portfolio as USDC in the OUSGInstantManager contract does not earn yield. USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function.

You can see that this talks about something else! and this issue dose not fall under this known issue.

Maybe a part of the mitigation suggest to use USDC leftover from other redemption to solve this issue, but the issue is here and the impact is clear. So, enough BUIDL balance is not preventing failed redemption if users redeem the full global redemption limit and this edge case occurs (which is proofed under PoC section)! it should be a sufficient BUIDL balance relative to global redemption limits where it counts this edge case and prevent it from happening.

Apologies for commenting at this point.

[radeveth]

Hello,

I appreciate the discussion and the points raised. However, after revisiting the README and the specific operational mechanisms of the OUSG Instant Redemption Manager, I maintain that the identified issue is invalid based on the intentional design and operational safeguards implemented by Ondo Finance. Here’s a detailed breakdown of why this bug is considered out of scope or a non-issue:

1. The Ondo Finance protocol is designed with specific operational checks that ensure the adequacy of BUIDL within the OUSGInstantManager. According to the protocol's operational procedures:

   "Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions."

   This statement implies a proactive management approach to maintaining BUIDL balances, which directly addresses concerns about potential redemption failures due to insufficient BUIDL.

2. The concerns raised about the potential for redemption failures when BUIDL balances are low seem to misinterpret the protocol's built-in safeguards. The protocol clearly outlines mechanisms for managing these situations:

   "USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function."

   This indicates that leftover USDC in the ousgManager contract is a considered part of the system's design, intended to handle smaller redemptions effectively without causing operational disruptions.

3. The phrase "may be utilized" does not imply exclusivity but rather flexibility in handling scenarios where BUIDL is temporarily insufficient. It provides a fallback mechanism to ensure that user redemptions can be processed even under constrained conditions. This flexibility is critical in managing the liquidity needs of users without compromising the integrity of the operational process.

4. The Proof of Concept provided seems to highlight a scenario that is already accounted for in the operational guidelines of the protocol. The test cases assume a situation that the protocol's management strategy is designed to prevent. Furthermore, the focus on this specific edge case does not reflect the typical or intended use case scenarios that the protocol is designed to handle.

Based on the detailed operational guidelines and the flexible management of BUIDL and USDC within the Ondo Finance protocol, the reported issue does not constitute a valid bug but rather a misunderstanding of the system's intended functionalities and safeguards.

Apologies for commenting at this point.

[Henrychang26]

Hi,

I would like to respectfully address the assessment provided for this issue.

Upon careful analysis and consideration, I believe there are aspects of this issue that distinguish it from issue #235, indicating that it is not a duplicate nor out of scope. Allow me to provide a detailed breakdown:

Issue Distinctions:

1. In the PoC of this issue, the Instant Manager contract has the minimum requirement for BUIDL redemption when Alice fails to redeem his share amount. With all respect, pre-sort evident is not true! instant Manager balance at the failed redeem transaction is 250_000e6 BUIDL. This line from the assertion BUIDL.balanceOf(address(ousgInstantManager)) + USDC.balanceOf(address(ousgInstantManager)) represents Alice's share after Bob's redemption, which is equivalent to 250_000e6 + 100e6.
2. In the primary issue it points the fail of the transaction in this require statement:

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

while in this issue, when Alice try to redeem his OUSG token the revert expected in the PoC specifically by an error that coming from BUIDL redeemer contract (Error:'Not enough tokens'):

 vm.expectRevert('Not enough tokens');
 ousgInstantManager.redeem(aliceOUSGRedeemAmount);

Clarification on Flow of Tokens From PoC:

This flow of tokens elucidates the sequence of events and the impact on user balances during redemption attempts from the PoC.

1. Starting with these balances:

Tokens Instant Manager Contract Balances Alice's Balance Bob's Balance OUSG 0 1,667 1,666 BUIDL 500,000 0 0 USDC 0 0 0

2. After Bob redeem his tokens:

Tokens Instant Manager Contract Balances Alice's Balance Bob's Balance OUSG 0 1,667 0 BUIDL 250,000 0 0 USDC 100 0 249,900

3. Now when Alice try to redeem his 1667e18 OUSG tokens will fail since the Instant Manager contract try to redeem BUIDL above its balance (250_100e6 BUIDL). However, Alice can only redeem an equivalent amount of OUSG less than or equal to the minimum requirement for BUIDL redemption.

In this PoC Alice only lost 100e6 USDC as it can not be redeemed, but it can be bigger as long as it is less than the minimum redemption requirement.

Given the distinctions outlined above and the implications for system functionality and user experience, I respectfully request a reconsideration of the categorization of this issue. It addresses a separate aspect of the contract's behavior and presents a valid concern that warrants attention.

Thank you for your time and consideration.

Warden seems to make the wrong assumption that as long as OUSGInstantManager holds at least minBUIDLRedeemAmount amount, redemption should be successful. This assumption is not explicitly stated anywhere in the documentation. minBUIDLRedeemAmount simply represents the minimum amount of BUIDL that must be redeemed in a single redemption.

The root cause of this case is the lack of sufficient BUIDL tokens in OUSGInstantManager, which falls the on the responsibility of the protocol. This was clearly noted in Automated Findings / Publicly Known Issues:

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.

Also protocol does not explicitly guarantee redemption if amount of BUIDL + USDC >= usdcAmountToRedeem since it acknowledges leftover USDC to be used at later time.

We are aware that USDC may be leftover and held within OUSGInstantManager when a user performs a redemption that is more than the minimumRedemption amount and less than the minimumBUIDLRedemption amount. We are also aware that this may cause temporary "cash drag" for the overall OUSG portfolio as USDC in the OUSGInstantManager contract does not earn yield. USDC may be utilized at a later time for servicing redemptions less than the minimumBUIDLRedemption amount OR retrieved through the permissioned retrieveTokens function.

Apologies for commenting at this point.

[c4-sponsor]

cameronclifton (sponsor) confirmed

[cameronclifton]

Due to changing requirements, the contract will now concatenate the USDC amount with BUIDL when performing redemptions. (This should mitigate this already known issue).