Skip to content
Request an audit

‹ All findings

Users will steal excess funds from the Vault due to VaultPoolLib::redeem() not always decreasing self.withdrawalPool.raBalance and self.withdrawalPool.paBalance

Crit/HighCork Protocol·Sherlock · Derivatives · 29th August 2024CodebaseH-6

Source

Summary

Vault::redeemExpiredLv() calls VaultLib::redeemExpired(), which allows users to withdraw funds after expiry, even if they have not requested a redemption. This redemption happens in VaultPoolLib::redeem(), when userEligible < amount, it calls internally __redeemExcessFromAmmPool(), where only self.ammLiquidityPool.balance is reduced, but not self.withdrawalPool.raBalance and self.withdrawalPool.paBalance. As such, when calculing the withdrawal pool balance in the next issuance on VaultPoolLibrary::reserve(), it will double count all the already withdraw self.withdrawalPool.raBalance and self.withdrawalPool.paBalance, allowing users to withdraw the same funds twice.

Root Cause

In VaultPoolLib::__redeemExcessFromAmmPool(), self.withdrawalPool.raBalance and self.withdrawalPool.paBalance are not decreased, but ra and pa are also withdrawn from the withdrawal pool when the user has partially requested redemption.

Internal pre-conditions

  1. User requests redemption of an amount smaller than the total withdrawn in VaultLib::redeemExpired(), that is, userEligible < amount.

External pre-conditions

None.

Attack Path

  1. User calls Vault::redeemExpiredLv(), withdrawing from the withdrawal pool, but self.withdrawalPool.raBalance and self.withdrawalPool.paBalance are not decreased.
  2. A new issuance starts, and in VaultPoolLib::reserve(), the funds are double counted as not all withdrawals were reduced.
  3. As such, self.withdrawalPool.raExchangeRate and self.withdrawalPool.paExchangeRate will be inflated by double the funds and users will redeem more funds than they should, leading to the insolvency of the Vault.

Impact

Users steal funds while unaware users will not be able to withdraw.

PoC

__tryRedeemExcessFromAmmPool() does not decrease the withdrawn self.withdrawalPool.raBalance and self.withdrawalPool.paBalance.

solidity
function __tryRedeemExcessFromAmmPool(VaultPool storage self, uint256 amountAttributed, uint256 excessAmount)
    internal
    view
    returns (uint256 ra, uint256 pa, uint256 withdrawnFromAmm)
{
    (ra, pa) = __tryRedeemfromWithdrawalPool(self, amountAttributed);

    withdrawnFromAmm =
        MathHelper.calculateRedeemAmountWithExchangeRate(excessAmount, self.withdrawalPool.raExchangeRate); //@audit PA is ignored here

    ra += withdrawnFromAmm;
}

Mitigation

Replace tryRedeemfromWithdrawalPool() with redeemfromWithdrawalPool().

solidity
function __tryRedeemExcessFromAmmPool(VaultPool storage self, uint256 amountAttributed, uint256 excessAmount)
    internal
    view
    returns (uint256 ra, uint256 pa, uint256 withdrawnFromAmm)
{
    (ra, pa) = __redeemfromWithdrawalPool(self, amountAttributed);

    withdrawnFromAmm =
        MathHelper.calculateRedeemAmountWithExchangeRate(excessAmount, self.withdrawalPool.raExchangeRate); //@audit PA is ignored here

    ra += withdrawnFromAmm;
}