Invalid handling of risdual amount in PositionAction::onCreditFlashLoan, forcing it to revert

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/main/src/proxy/PositionAction.sol#L492-L499

Vulnerability details

Description

Users can call PositionAction::decreaseLever through a proxy, to "Decrease the leverage of a position by taking out a credit flash loan to withdraw and sell collateral", after it is called, the flash loan lender sends the credit and calls onCreditFlashLoan, which handles all that logic. When doing so, users are supposed to swap their collateral withdrawn into debt tokens so that the flash loan can be repaid.

The protocol tries to handle the residual amount from the swap (swapped - paid debt), by trying to repay extra debt for the designated position, using:

if (residualAmount > 0) {
    underlyingToken.forceApprove(address(leverParams.vault), residualAmount);
    ICDPVault(leverParams.vault).modifyCollateralAndDebt(
        leverParams.position,
        address(this),
        address(this),
        0,
        -toInt256(residualAmount)
    );
}

However, this is invalid for 2 main reasons:

1. This is trying to repay extra debt than what the user is trying to, which is passed in the primarySwap.amount.
2. If the user tries to repay his whole debt using decreaseLever the TX will revert, as it'll try to repay some nonexistent debt.

Proof of Concept

Add the following test in src/test/integration/PositionAction20.lever.t.sol:

function test_leverageDownWrongResidualHandling() public {
    uint256 depositAmount = 1_000 ether;
    uint256 borrowAmount = 200 ether;

    deal(address(token), user, depositAmount);

    address[] memory assets = new address[](2);
    assets[0] = address(token);
    assets[1] = address(underlyingToken);

    vm.startPrank(user);

    // User deposits 1k ETH collateral
    token.approve(address(vault), type(uint256).max);
    vault.deposit(address(userProxy), depositAmount);

    // User borrows 200 ETH
    userProxy.execute(
        address(positionAction),
        abi.encodeWithSelector(
            positionAction.borrow.selector,
            address(userProxy),
            address(vault),
            CreditParams({amount: borrowAmount, creditor: user, auxSwap: emptySwap})
        )
    );

    userProxy.execute(
        address(positionAction),
        abi.encodeWithSelector(
            positionAction.decreaseLever.selector,
            LeverParams({
                position: address(userProxy),
                vault: address(vault),
                collateralToken: address(token),
                primarySwap: SwapParams({
                    swapProtocol: SwapProtocol.BALANCER,
                    swapType: SwapType.EXACT_IN,
                    assetIn: address(token),
                    amount: vault.virtualDebt(address(userProxy)),
                    limit: 0,
                    recipient: address(positionAction),
                    residualRecipient: address(positionAction),
                    deadline: block.timestamp,
                    args: abi.encode(weightedPoolIdArray, assets)
                }),
                auxSwap: emptySwap,
                auxAction: emptyPoolActionParams
            }),
            201 ether,
            address(userProxy)
        )
    );

    vm.stopPrank();
}

Logs:

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.65s (4.18ms CPU time)

Ran 1 test suite in 2.66s (2.65s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/test/integration/PositionAction20.lever.t.sol:PositionAction20_Lever_Test
[FAIL. Reason: CallerNotCreditManagerException()] test_leverageDownWrongResidualHandling() (gas: 1088990)

Recommended Mitigation Steps

Rather than using the residual amount to repay excess debt (that might not even exist), transfer it to the designated residual recipient. Alternatively, check if the user still has any remaining debt. If they do, use the residual to repay it; otherwise, transfer the residual amount to the recipient.

Assessed type

DoS

[amarcu]

The error in the failing test if because of a faulty setup. This will happen if the flashlender contract is not added to the poolv3 as a credit manager.

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid

[0xAlix2]

@koolexcrypto - I believe there's confusion here, the error here isn't because "the flashlender contract is not added to the poolv3 as a credit manager", please let me explain:

In https://github.com/code-423n4/2024-10-loopfi/blob/main/src/PoolV3.sol, if you search for CallerNotCreditManagerException you can find 2 occurrences, the first is here:

function _revertIfCallerNotCreditManager() internal view {
    if (!_creditManagerSet.contains(msg.sender)) {
        revert CallerNotCreditManagerException(); // U:[PQK-4]
    }
}

This is indeed the case that the sponsor is referencing, however, there's another occurrence here:

if (cmBorrowed == 0) {
    revert CallerNotCreditManagerException(); // U:[LP-2C,14A]
}

This is the case that the report is discussing, where this error is thrown when the borrowed of a position equals 0.

You can easily confirm this by changing the amount passed to amount in the above PoC to a lower value and see that the test doesn't fail, example below:

function test_leverageDownWrongResidualHandling() public {
    uint256 depositAmount = 1_000 ether;
    uint256 borrowAmount = 200 ether;

    deal(address(token), user, depositAmount);

    address[] memory assets = new address[](2);
    assets[0] = address(token);
    assets[1] = address(underlyingToken);

    vm.startPrank(user);

    // User deposits 1k ETH collateral
    token.approve(address(vault), type(uint256).max);
    vault.deposit(address(userProxy), depositAmount);

    // User borrows 200 ETH
    userProxy.execute(
        address(positionAction),
        abi.encodeWithSelector(
            positionAction.borrow.selector,
            address(userProxy),
            address(vault),
            CreditParams({amount: borrowAmount, creditor: user, auxSwap: emptySwap})
        )
    );

    userProxy.execute(
        address(positionAction),
        abi.encodeWithSelector(
            positionAction.decreaseLever.selector,
            LeverParams({
                position: address(userProxy),
                vault: address(vault),
                collateralToken: address(token),
                primarySwap: SwapParams({
                    swapProtocol: SwapProtocol.BALANCER,
                    swapType: SwapType.EXACT_IN,
                    assetIn: address(token),
-                   amount: vault.virtualDebt(address(userProxy)),
+                   amount: vault.virtualDebt(address(userProxy)) / 2,
                    limit: 0,
                    recipient: address(positionAction),
                    residualRecipient: address(positionAction),
                    deadline: block.timestamp,
                    args: abi.encode(weightedPoolIdArray, assets)
                }),
                auxSwap: emptySwap,
                auxAction: emptyPoolActionParams
            }),
            201 ether,
            address(userProxy)
        )
    );

    vm.stopPrank();
}

Hence, I believe there's some confusion here and this is a valid medium and would appreciate if you could take another look.

[koolexcrypto]

@amarcu

I have run the PoC and changed the error

// src/PoolV3.sol : L588
        if (cmBorrowed == 0) {
            revert CallerNotCreditManagerException(); // U:[LP-2C,14A]
        }

to other a different one. When running the PoC, it throws this error. This confirms the error is caused when there is no debt left cmBorrowed == 0

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report