PositionAction4626.sol#_onWithdraw should withdraw from position CDPVault position instead of address(this).

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/main/src/proxy/PositionAction4626.sol#L67

Vulnerability details

Impact

PositionAction4626.sol#_onWithdraw does not withdraw from the correct position - it should withdraw from position instead of address(this).

Bug Description

Note: This is based on the 2024-07 Loopfi contest code-423n4/2024-07-loopfi-findings#81, code-423n4/2024-07-loopfi-findings#80 issue. This protocol team applied a fix, but the fix is incomplete.

Only the bug in the _onDeposit() was fixed, but not the one in _onWithdraw().

PositionAction4626.sol#_onWithdraw does not withdraw from the correct position - it should withdraw from position instead of address(this).

    function _onDeposit(address vault, address position, address src, uint256 amount) internal override returns (uint256) {
        address collateral = address(ICDPVault(vault).token());

        // if the src is not the collateralToken, we need to deposit the underlying into the ERC4626 vault
        if (src != collateral) {
            address underlying = IERC4626(collateral).asset();
            IERC20(underlying).forceApprove(collateral, amount);
            amount = IERC4626(collateral).deposit(amount, address(this));
        }

        IERC20(collateral).forceApprove(vault, amount);

        // @audit-note: This was fixed.
        return ICDPVault(vault).deposit(position, amount);
    }


    function _onWithdraw(
        address vault,
        address /*position*/,
        address dst,
        uint256 amount
    ) internal override returns (uint256) {
        // @audit-note: This is still a bug.
@>      uint256 collateralWithdrawn = ICDPVault(vault).withdraw(address(this), amount);

        // if collateral is not the dst token, we need to withdraw the underlying from the ERC4626 vault
        address collateral = address(ICDPVault(vault).token());
        if (dst != collateral) {
            collateralWithdrawn = IERC4626(collateral).redeem(collateralWithdrawn, address(this), address(this));
        }

        return collateralWithdrawn;
    }

Proof of Concept

N/A

Tools Used

Manual Review

Recommended Mitigation Steps

-       uint256 collateralWithdrawn = ICDPVault(vault).withdraw(address(this), amount);
+       uint256 collateralWithdrawn = ICDPVault(vault).withdraw(position, amount);

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report