CollateralTracker lacks slippage protection in redeem() and withdraw() functions

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L591-L626
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L531-L566

Vulnerability details

Description

Users (who previously deposited assets) can withdraw and redeem their assets via functions withdraw() or redeem() at 1:1 ratio on a normal circumstance. Meaning, they are expecting an amount of at least equal to the amount that they have deposited.

However there will be occasions where their assets "withdrawals / redemptions" might be less than what they originally deposited. That scenario happens when the vault experiences losses.

• redeem()

  function redeem(
      uint256 shares,
      address receiver,
      address owner
  ) external returns (uint256 assets) {
      if (shares > maxRedeem(owner)) revert Errors.ExceedsMaximumRedemption();
  
      // check/update allowance for approved redeem
      if (msg.sender != owner) {
          uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
  
          if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
      }
  
      assets = previewRedeem(shares); <<@ loss happens here
  
      // burn collateral shares of the Panoptic Pool funds (this ERC20 token)
      _burn(owner, shares);
  
      // update tracked asset balance
      unchecked {
          s_poolAssets -= uint128(assets);
      }
  
      // transfer assets (underlying token funds) from the PanopticPool to the LP
      SafeTransferLib.safeTransferFrom(
          s_underlyingToken,
          address(s_panopticPool),
          receiver,
          assets
      );
  
      emit Withdraw(msg.sender, receiver, owner, assets, shares);
  
      return assets;
  }

  • previewRedeem()

  function previewRedeem(uint256 shares) public view returns (uint256 assets) {
      return convertToAssets(shares);
  }

  • convertToAssets()

  function convertToAssets(uint256 shares) public view returns (uint256 assets) {
      return Math.mulDiv(shares, totalAssets(), totalSupply); 
  }

Looking at the math expression above shares * totalAssets() / totalSupply(), when totalAssets() decreases, the return value (assets) also decreases.

Here's a scenario where the losses happen:

• Suppose the vault is operating on a normal condition (without a loss).
• Alice previously deposited an asset amount of 1000 and received 1000 amount of shares in return (1:1).
• Now Alice is about to redeem the full amount and is expecting 1000 amount of assets in exchange of her 1000 shares.
• While Alice's transaction is still pending in the mempool, the vault suddenly experience losses and the totalAssets() dropped by 10% below the original ratio of 1:1.
• Now the transaction went through, she received 900 amount of assets (less than 10% of what she is expecting).
• At this point, Alice realized her losses.

If only Alice knew that this will happen, she might have opted to wait until the assets goes back to at least 1:1 ratio to at least redeem back her original deposit amount. Without slippage protection, Alice has to suffer these losses.

This principle also applies to withdraw() but with a slight difference.

In redeem() the input (fixed) amount of shares is exchanged with assets. When losses occur, these fixed amount shares will produce lesser assets.

In withdraw() the input (fixed) amount of assets will cause to burn more shares to get the same asset amount which also in itself a loss.

• withdraw()

  function withdraw(
      uint256 assets,
      address receiver,
      address owner
  ) external returns (uint256 shares) {
      if (assets > maxWithdraw(owner)) revert Errors.ExceedsMaximumRedemption();
  
      shares = previewWithdraw(assets);
  
      // check/update allowance for approved withdraw
      if (msg.sender != owner) {
          uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
  
          if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
      }
  
      // burn collateral shares of the Panoptic Pool funds (this ERC20 token)
      _burn(owner, shares);
  
      // update tracked asset balance
      unchecked {
          s_poolAssets -= uint128(assets);
      }
  
      // transfer assets (underlying token funds) from the PanopticPool to the LP
      SafeTransferLib.safeTransferFrom(
          s_underlyingToken,
          address(s_panopticPool),
          receiver,
          assets
      );
  
      emit Withdraw(msg.sender, receiver, owner, assets, shares);
  
      return shares;
  }

  • previewWithdraw()

  function previewWithdraw(uint256 assets) public view returns (uint256 shares) {
      uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
  
      return Math.mulDivRoundingUp(assets, supply, totalAssets());
  }

Looking at the math expression above assets * supply / totalAssets(), when totalAssets() decreases, the return value (shares) increases pressuring to burn more shares.

_burn(owner, shares);

Impact

Without slippage protection on redeem() and withdraw() functions, users will lose funds in the event of vault loss that suddenly happens in the middle of the redeem / withdraw transactions.

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a function with the same name redeem / withdraw (to still be compliant with ERC4626 but with different set of parameters) but add parameters minAssets and maxShares respectively.

• For redeem()

  function redeem(
      uint256 shares,
      address receiver,
      address owner
  ) external returns (uint256 assets) {
      ...
  }
  
  + function redeem(
  +     uint256 shares,
  +     address receiver,
  +     address owner
  +     uint256 minAssets
  + ) external returns (uint256 assets) {
  +     assets = redeem(shares, receiver, owner);
  +     if (assets < minAssets) revert Errors.AssetsTooLow();
  + }

• For withdraw()

  function withdraw(
      uint256 assets,
      address receiver,
      address owner
  ) external returns (uint256 shares) {
      ...
  }
  
  + function withdraw(
  +    uint256 assets,
  +    address receiver,
  +    address owner,
  +    uint256 maxShares
  + ) external override returns (uint256 shares) {
  +    shares = withdraw(assets, receiver, owner);
  +    if (shares > maxShares) revert Errors.SharesTooHigh();
  + }

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #365

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)