KupiaSec - The `DsFlashSwap.emptyReserve()` function incorrectly always returns 0

[sherlock-admin2]

KupiaSec

High

The DsFlashSwap.emptyReserve() function incorrectly always returns 0

Summary

The DsFlashSwap.emptyReserve() function incorrectly returns 0 instead of the original reserve amount. This leads to erroneous behavior within the VaultLib._liquidatedLp() function, which relies on the DsFlashSwap.emptyReserve() function, as it should utilize the original reserve amount.

Vulnerability Detail

As noted at line 77, the DsFlashSwap.emptyReservePartial() function returns the remaining reserve amount, not the removed amount. Consequently, the DsFlashSwap.emptyReserve() function always returns 0 (see line 67), as it removes the entire reserve amount. This, in turn, causes the FlashSwapRouter.emptyReserve() function to also return 0, since it calls the DsFlashSwap.emptyReserve() function at line 70.

DsFlashSwap.sol

    function emptyReserve(ReserveState storage self, uint256 dsId, address to) internal returns (uint256 reserve) {
67      reserve = emptyReservePartial(self, dsId, self.ds[dsId].reserve, to);
    }

    function emptyReservePartial(ReserveState storage self, uint256 dsId, uint256 amount, address to)
        internal
        returns (uint256 reserve)
    {
        self.ds[dsId].ds.transfer(to, amount);

        self.ds[dsId].reserve -= amount;
77      reserve = self.ds[dsId].reserve;
    }

------------------------

FlashSwapRouter.sol

    function emptyReserve(Id reserveId, uint256 dsId) external override onlyOwner returns (uint256 amount) {
70      amount = reserves[reserveId].emptyReserve(dsId, owner());
        emit ReserveEmptied(reserveId, dsId, amount);
    }

This results in incorrect behavior within the VaultLib._liquidatedLp() function.

For instance, at line 374 of the VaultLib._liquidatedLp() function, the value of reservedDs is always 0 because the flashSwapRouter.emptyReserve() function consistently returns 0. This impacts redeemAmount, making it always 0 (see line 376), which leads to incomplete redemption of RA. Consequently, users will incur a loss of RA.

    function _liquidatedLp(
        ...

374     uint256 reservedDs = flashSwapRouter.emptyReserve(self.info.toId(), dsId);

376     uint256 redeemAmount = reservedDs >= ctAmm ? ctAmm : reservedDs;
        PsmLibrary.lvRedeemRaWithCtDs(self, redeemAmount, dsId);

        ...

Impact

Leading to incomplete redemption of RA when processing expired states, resulting in a loss of RA for users.

Code Snippet

https://github.com/sherlock-audit/2024-08-cork-protocol/tree/main/Depeg-swap/contracts/libraries/DsFlashSwap.sol#L66-L78

https://github.com/sherlock-audit/2024-08-cork-protocol/tree/main/Depeg-swap/contracts/core/flash-swaps/FlashSwapRouter.sol#L69-L72

https://github.com/sherlock-audit/2024-08-cork-protocol/tree/main/Depeg-swap/contracts/libraries/VaultLib.sol#L349-L393

Tool used

Manual Review

Recommendation

Make the following fixes.

    function emptyReserve(ReserveState storage self, uint256 dsId, address to) internal returns (uint256 reserve) {
-       reserve = emptyReservePartial(self, dsId, self.ds[dsId].reserve, to);
+       reserve = self.ds[dsId].reserve;
+       emptyReservePartial(self, dsId, self.ds[dsId].reserve, to);
    }

Duplicate of #68