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

[sherlock-admin2]

0x73696d616f

High

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

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.

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().

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

[ziankork]

This is a valid issue, though we removed the functionality of expiry redemption on the LV, so this would have no impact on the updated version of the protocol. still valid nonetheless