Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c4-bot-3 opened this issue Oct 12, 2024 · 0 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_14_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-3
Copy link
Contributor

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

@c4-bot-3 c4-bot-3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 12, 2024
c4-bot-8 added a commit that referenced this issue Oct 12, 2024
@c4-bot-12 c4-bot-12 added the 🤖_14_group AI based duplicate group recommendation label Oct 18, 2024
howlbot-integration bot added a commit that referenced this issue Oct 19, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_14_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants