c4-bot-6 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/Liquidizer.sol#L101-L126 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L115-L135

Vulnerability details

Impact

When users repay their borrowed USDS, those tokens are sent to the USDS token address rather than the Liquidizer contract. However, the Liquidizer contract expects this USDS to be part of its balance, and since its not, it has logic to withdraw POL in order to swap into USDS. This results in a direct loss of assets for the DAO due to loss of POL, due to the burning of an improper amount of USDS.

Proof of Concept

When a user is repaying their USDS 'loan', they will call CollateralAndLiquidity:repayUSDS, which is defined as follows:

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

  ...
  }

Most importantly here, the USDS transferred from the user is send to the USDS token address, while incrementBurnableUSDS is called on the liquidizer contract, which increments the usdsThatShouldBeBurned state variable. This function call also occurs in the CollateralAndLiquidity:liquidateUser function where it records the total USDS borrow balance which needs to be acquired through swapping the taken user collateral.

Let's now see how this variable is used in the Liquidizer contract. It has the Liquidizer:performUpkeep function which is tasked with swapping the taken collateral (e.g. WETH, WBTC) into USDS, then burning the correct amount of USDS. As part of this call it calls the internal function _possiblyBurnUSDS which handles the logic for burning USDS, which is defined as follows:

function _possiblyBurnUSDS() internal
   {
    ...
   uint256 usdsBalance = usds.balanceOf(address(this));
@>   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;
      }
   else
      {
      // The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
      _burnUSDS( usdsBalance );
      usdsThatShouldBeBurned -= usdsBalance;

      // As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
      // send the underlying tokens here to be swapped to USDS
@>      dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
@>      dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);
      }
   }

Notice that comparing the current balance of USDS against the usdsThatShouldBeBurned state variable is how this logic decides whether to remove POL. And recall earlier that when a user repays USDS, those tokens are sent to the USDS contract rather than to the Liquidizer contract, while usdsThatShouldBeBurned is still incremented by the sent amount. This is invalid as there is now a larger usdsThatShouldBeBurned value than there should be USDS in this contract. This in turn means that POL is being removed incorrectly, which results in a loss of assets for the DAO.

Tools Used

Manual review

Recommended Mitigation Steps

In the repayUSDS function call, USDS should be sent to the Liquidizer contract rather than to the USDS token address.

Assessed type

Other

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #618

c4-judge commented 6 months ago

Picodes marked the issue as satisfactory