Excess burning of USDC

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125-L128

Vulnerability details

Impact

Users have the liberty to borrow USDS by providing overcollaterized asset(WBTC/WETH), this asset can with returned back when the user repays the loan, when a user defaults, the user asset are then liquidated and converted to USDS to be burnt.

In the case of liquidation, after another user helps to liquidate an underwater loan for a commission, the asset are sent to Liquidizer contract, when collateralAndLiquidity.liquidizer().performUpkeep(); is called, the liquidizer contract sells the required collaterals that have been sent to convert them to USDS, then calls _possiblyBurnUSDS(); to burn all usdsThatShouldBeBurned

    // This corresponds to USDS that was borrowed by users who had their collateral liquidated.
    // Because liquidated collateral no longer exists, the borrowed USDS needs to be burned in order to "undo" the collateralized position
    // and return state back to where it was before the liquidated user deposited collateral and borrowed USDS.
    uint256 public usdsThatShouldBeBurned;

as stated by the comment above usdsThatShouldBeBurned corresponds to usds that was borrowed by users that had their collateral liquidated.

The problem here is that users that did not have their collateral liquidated also increases the value. This should not be so because in repayUSDS() borrowed USDS have been transferred to USDS contract to be burnt internally

// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
// Have USDS remember that the USDS should be burned
liquidizer.incrementBurnableUSDS( amountRepaid );

The implication will be that, the liquidizer will burn excess USDS than it should

Proof of Concept

Let us consider a case of two borrowers; Alice and Bob

1. Alice and Bob takes a loan of 1_000_000 USDS each with the right amount of shares to keep the loan overcollaterized.
2. Alice calls repayUSDS() to pay back all the debt, 1_000_000 USDS was sent to USDS contract and usdsThatShouldBeBurned was still incremented by same amount. So USDS balance in USDS contract is 1,000,000 and usdsThatShouldBeBurned is 1,000,000

   // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
   usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);
   // Have USDS remember that the USDS should be burned
   liquidizer.incrementBurnableUSDS( amountRepaid );

3. Bob's loan on the other hand became undercollateralized and due for liquidation.
4. Greg then called the liquidateUser() to liquidate Bob, Bob's collaterals are pulled out and sent to the Liquidizer contract, and usdsThatShouldBeBurned is increased by another 1,000,000 to make it 2,000,000
5. When collateralAndLiquidity.liquidizer().performUpkeep(); was called, required amount of previously sent tokens are swapped for USDS, total from the sales of WETH, WBTC and DAI made USDS balance of the Liquidizer to be 2,000,000.

6. When _possiblyBurnUSDS(); was then called, _burnUSDS( 2,000,000 ) will lead to transferring the 2,000,000 USDS to the USDS contract(Note: this should have been 1,000,000 since the Alice USDS was previously sent to the USDS contract as soon as repayment was done)

   // Burn the specified amount of USDS
   function _burnUSDS(uint256 amountToBurn) internal
       {
       usds.safeTransfer( address(usds), amountToBurn );
       usds.burnTokensInContract();
       }

   Now let see what happens in usds.burnTokenInContract()

   
   // USDS tokens will need to be sent here before they are burned (on a repayment or liquidation).
   // There should otherwise be no USDS balance in this contract.
   function burnTokensInContract() external
       {
       uint256 balance = balanceOf( address(this) );
       _burn( address(this), balance );
   
       emit USDSTokensBurned(balance);
       }
   }

From the code snippet above, we can see that when balance is checked, USDS contract will have 3,000,000 USDS instead of 2,000,000 and all will be burnt, burning excess of 1,000,000 USDS.
Meanwhile the excess 1,000,000 USDS should have been left in the Liquidizer contract should incase of when there is shortage of USDS and liquidated amount need to be balanced
``` solidity
        if ( usdsBalance >= usdsThatShouldBeBurned )
            {
            // Burn only up to usdsThatShouldBeBurned.
            // Leftover USDS will be kept in this contract in case it needs to be burned later.
            _burnUSDS( usdsThatShouldBeBurned );
            usdsThatShouldBeBurned = 0;
            }

Tools Used

Manual review

Recommended Mitigation Steps

// Repay borrowed USDS and adjust the user's usdsBorrowedByUser
function repayUSDS( uint256 amountRepaid ) external nonReentrant
    {
require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
require( amountRepaid <= usdsBorrowedByUsers[msg.sender], "Cannot repay more than the borrowed amount" );
require( amountRepaid > 0, "Cannot repay zero amount" );

// Decrease the borrowed amount for the user
usdsBorrowedByUsers[msg.sender] -= amountRepaid;
// Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
    usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

// Have USDS remember that the USDS should be burned
--  liquidizer.incrementBurnableUSDS( amountRepaid ); //@audit remove this
++      usds.burnTokensInContract(); 

// Check if the user no longer has any borrowed USDS
if ( usdsBorrowedByUsers[msg.sender] == 0 )
_walletsWithBorrowedUSDS.remove(msg.sender);

emit RepaidUSDS(msg.sender, amountRepaid);
    }

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #240

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #137

[c4-judge]

Picodes changed the severity to 3 (High Risk)