code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L721-L724

Vulnerability details

Not resetting totalBurned in CashManger will break user redemptions

The current implementation in CashManager.completeRedemptions is not updating the totalBurned amount in an epoch if there was a refund. The problem is, that if not all user redemptions can be processed in a single call per epoch it will break the withdraws for all users that are processed in the second call for the epoch as they will receive less tokens.

Scenario:

There are lots of requestRedemption's from users in an epoch.
For all the requestRedemption there is at least one that will receive a refund
As there are too much requestRedemption's to process all in one transaction (or any other reason why the admin want to split it), the admin needs to schedule another transaction with the rest of the users

The users in the second transaction will now get less tokens than the users processed in the first transaction.

Proof of Concept

The following test should succeed, but Bob only gets 130666666 instead of 147000000 tokens and so it fails.

Add Test to File: forge-tests/cash/cash_manager/Redemption.t.sol

function test_redeem_redeemRefund_multiple() public {
    // Seed alice and bob with 200 cash tokens
    _seed(200e18, 200e18, 50e18);

    // Have alice request to withdraw 200 cash tokens
    vm.startPrank(alice);
    tokenProxied.approve(address(cashManager), 200e18);
    cashManager.requestRedemption(200e18);
    vm.stopPrank();

    // Have bob request to withdraw 200 cash tokens
    vm.startPrank(bob);
    tokenProxied.approve(address(cashManager), 200e18);
    cashManager.requestRedemption(200e18);
    vm.stopPrank();

    // Have charlie request to withdraw his tokens
    vm.startPrank(charlie);
    tokenProxied.approve(address(cashManager), 50e18);
    cashManager.requestRedemption(50e18);
    vm.stopPrank();

    // Move forward to the next epoch
    vm.warp(block.timestamp + 1 days);
    vm.prank(managerAdmin);
    cashManager.setMintExchangeRate(2e6, 0);

    // Approve the cashMinter contract from the assetSender account
    _seedSenderWithCollateral(400e6);

    // Airdrop Alice and bob their collateral
    toWithdraw.push(alice);    

    // Issue charlie a refund for whatever reason
    toRefund.push(charlie);

    vm.prank(managerAdmin);
    cashManager.completeRedemptions(
      toWithdraw, // Addresses to issue collateral to
      toRefund, // Addresses to refund cash
      300e6, // Total amount of money to dist incl fees
      0, // Epoch we wish to process
      6e6 // Fee amount to be transferred to ondo
    );

    assertEq(USDC.balanceOf(alice), 147e6);    
    assertEq(tokenProxied.balanceOf(charlie), 50e18);
    toRefund.pop(); // cleanup
    toWithdraw.pop(); // cleanup

    // second redemption call as we had to much in first call and need another transaction to fulfill the rest of the users for the epcoh
    toWithdraw.push(bob);

    vm.prank(managerAdmin);
    cashManager.completeRedemptions(
      toWithdraw, // Addresses to issue collateral to
      toRefund, // Addresses to refund cash
      300e6, // Total amount of money to dist incl fees
      0, // Epoch we wish to process
      6e6 // Fee amount to be transferred to ondo
    );

    assertEq(USDC.balanceOf(alice), 147e6); // not changed    
    assertEq(tokenProxied.balanceOf(charlie), 50e18); // not changed

    assertEq(USDC.balanceOf(bob), 147e6); // we expect the same result for bob that alice got
}

Tools Used

Manual review

Recommended Mitigation Steps

Update the totalBurned amount for the epoch after the refunds.

File: contracts/cash/CashManager.sol
721:     uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
722:       .totalBurned - refundedAmt;
723: 
724:     // @audit update the epoch totalBurned amount
725:     redemptionInfoPerEpoch[epochToService].totalBurned = quantityBurned;
726:     
727:     uint256 amountToDist = collateralAmountToDist - fees;

c4-judge commented 1 year ago

trust1995 marked the issue as duplicate of #325