Inadequate update of repayment amount post-swap

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L589
https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L575

Vulnerability details

Inadequate update of repayment amount post-swap

Lines of code

• https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L589
• https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L575

Impact

In the PositionAction contract, specifically at PositionAction#L589, the repayment amount used in the debt modification process does not account for potential changes resulting from swap operations. Instead, it uses the initial creditParams.amount, which may not reflect the correct amount after a swap. This discrepancy can lead to incorrect debt repayment calculations, causing financial inconsistencies and potential losses for users due partial repayments.

    function _repay(address vault, address position, CreditParams calldata creditParams, PermitParams calldata permitParams) internal {
        // transfer arbitrary token and swap to underlying token
        uint256 amount;
        if (creditParams.auxSwap.assetIn != address(0)) {
            if (creditParams.auxSwap.recipient != address(this)) revert PositionAction__repay_InvalidAuxSwap();

>>>         amount = _transferAndSwap(creditParams.creditor, creditParams.auxSwap, permitParams);
        } else {
            if (creditParams.creditor != address(this)) {
                // transfer directly from creditor
                _transferFrom(address(underlyingToken), creditParams.creditor, address(this), creditParams.amount, permitParams);
            }
        }

>>>     underlyingToken.forceApprove(address(vault), creditParams.amount);
        ICDPVault(vault).modifyCollateralAndDebt(
            position,
            address(this),
            address(this),
            0,
>>>         -toInt256(creditParams.amount)
        );
    }

In this code, the amount variable from PositionAction#L575 is intended to capture the correct repayment amount after any swap operation. However, the subsequent forceApprove and modifyCollateralAndDebt calls at PositionAction#L583 and PositionAction#L589, respectively, use creditParams.amount instead of the updated amount. This oversight can lead to incorrect repayment amounts being processed.

When the swap returns an amount of underlying greater than creditParams.amount, it will result in a remaining amount left in the contract because the call to modifyCollateralAndDebt will use creditParams.amount. Conversely, when the amount returned from the swap is less than creditParams.amount, the repayment transaction will be reversed.

Proof of Concept

The following test demonstrates how the contract retains the unused amount from the swap when the swap is executed and the underlying is greater than creditParams.amount.

   function test_repayment_with_swap_to_underlying_is_more_than_creditparamsamount() public {
        uint256 depositAmount = 1_000 * 1 ether;
        uint256 borrowAmount = 500 * 1 ether;
        //
        // 1. Deposit collateral and borrow
        _depositAndBorrow(userProxy, address(vault), depositAmount, borrowAmount);
        //
        // 2. Build repay params with `borrowAmount` amount
        uint256 repaymentAmount = borrowAmount;
        uint256 swapAdjustedMaxIn = repaymentAmount * 102 / 100;
        deal(address(token), user, swapAdjustedMaxIn);
        bytes32[] memory poolIds = new bytes32[](1);
        poolIds[0] = weightedUnderlierPoolId;
        address[] memory assets = new address[](2);
        assets[0] = address(underlyingToken);
        assets[1] = address(token);
        CreditParams memory creditParams = CreditParams({
            amount: repaymentAmount,
            creditor: user,
            auxSwap: SwapParams({
                swapProtocol: SwapProtocol.BALANCER,
                swapType: SwapType.EXACT_OUT,
                assetIn: address(token),
                amount: repaymentAmount, // amount to swap in
                limit: swapAdjustedMaxIn, // max amount of collateral token to receive
                recipient: address(userProxy),
                deadline: block.timestamp + 100,
                args: abi.encode(poolIds, assets)
            })
        });
        _simulateBalancerSwap(creditParams.auxSwap);
        //
        // 3. Repay the debt using the `repaymentAmount`.
        vm.startPrank(user);
        token.approve(address(userProxy), swapAdjustedMaxIn);
        userProxy.execute(
            address(positionAction),
            abi.encodeWithSelector(
                positionAction.repay.selector,
                address(userProxy), // user proxy is the position
                address(vault),
                creditParams,
                emptyPermitParams
            )
        );
        vm.stopPrank();
        //
        // 4. Assert conditions to ensure repayment amount is updated correctly
        //    The problem is that not all of the swap amount is used, leaving a remainder inside the contract.
        (uint256 collateral, uint256 debtAfter, , , , ) = vault.positions(address(userProxy));
        assertEq(collateral, depositAmount);
        assertEq(debtAfter, 0);
        assertEq(underlyingToken.balanceOf(user), 5e20); // remaining amount inside contract
    }

The following test shows how the repayment transaction is reversed when, after the swap, the amount of underlying is less than creditParams.amount.

function test_repayment_with_swap_to_underlying_is_less_than_creditparamsamount() public {
        uint256 depositAmount = 1_000 * 1 ether;
        uint256 borrowAmount = 500 * 1 ether;
        //
        // 1. Deposit collateral and borrow
        _depositAndBorrow(userProxy, address(vault), depositAmount, borrowAmount);
        //
        // 2. Build repay params with `borrowAmount` amount
        uint256 repaymentAmount = borrowAmount;
        deal(address(token), user, repaymentAmount);
        uint256 swapAdjustedMinOut = repaymentAmount * 99 / 100;
        bytes32[] memory poolIds = new bytes32[](1);
        poolIds[0] = weightedUnderlierPoolId;
        address[] memory assets = new address[](2);
        assets[0] = address(token);
        assets[1] = address(underlyingToken);
        CreditParams memory creditParams = CreditParams({
            amount: repaymentAmount,
            creditor: user,
            auxSwap: SwapParams({
                swapProtocol: SwapProtocol.BALANCER,
                swapType: SwapType.EXACT_IN,
                assetIn: address(token),
                amount: repaymentAmount, // amount to swap in
                limit: swapAdjustedMinOut, // min amount of collateral token to receive
                recipient: address(userProxy),
                deadline: block.timestamp + 100,
                args: abi.encode(poolIds, assets)
            })
        });
        _simulateBalancerSwap(creditParams.auxSwap);
        //
        // 3. Repay the debt using the `repaymentAmount`. The tx will be reverted.
        vm.startPrank(user);
        token.approve(address(userProxy), repaymentAmount);
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        userProxy.execute(
            address(positionAction),
            abi.encodeWithSelector(
                positionAction.repay.selector,
                address(userProxy), // user proxy is the position
                address(vault),
                creditParams,
                emptyPermitParams
            )
        );
        vm.stopPrank();
    }

Tools used

Manual review

Recommended Mitigation Steps

To mitigate this issue and ensure accurate debt repayment calculations, the code should be updated to use the amount variable, which reflects the actual amount post-swap. This change will ensure that the repayment process correctly accounts for any adjustments resulting from swaps.

function _repay(address vault, address position, CreditParams calldata creditParams, PermitParams calldata permitParams) internal {
    // transfer arbitrary token and swap to underlying token
    uint256 amount;
    if (creditParams.auxSwap.assetIn != address(0)) {
        if (creditParams.auxSwap.recipient != address(this)) revert PositionAction__repay_InvalidAuxSwap();

        amount = _transferAndSwap(creditParams.creditor, creditParams.auxSwap, permitParams);
    } else {
        if (creditParams.creditor != address(this)) {
            // transfer directly from creditor
            _transferFrom(address(underlyingToken), creditParams.creditor, address(this), creditParams.amount, permitParams); // L579
+           amount = creditParams.amount; // Ensure amount is set correctly
        }
    }

-   underlyingToken.forceApprove(address(vault), creditParams.amount);
+   underlyingToken.forceApprove(address(vault), amount); // L583
    ICDPVault(vault).modifyCollateralAndDebt(
        position,
        address(this),
        address(this),
        0,
-       -toInt256(creditParams.amount)
+       -toInt256(amount) // L586
    );

Assessed type

Context

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as duplicate of #526