Wrong repayment amount used in PositionAction::_repay, forcing users to unexpectedly lose funds

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/proxy/PositionAction.sol#L569-L591

Vulnerability details

Impact

PositionAction allows users to interact with their position in the CDP Vault through a proxy, on top of that it allows users to do certain actions before interacting with the position.
An example of this is the PositionAction::deposit, which allows users to:

1. Deposit collateral tokens directly into the position
2. Swap arbitrary tokens for collateral and the deposit into the position

This is handled in PositionAction::_deposit, where if swap params exist, swap takes place and the returned amount is used when depositing into the position, else the user's specified amount is used.

However, in PositionAction::_repay, this is not the case, where even if a swap took place, the amount sent to the vault/position is still the one specified by the user, which is wrong and inconsistent with the other functions' API.

This can cause unexpected behaviors and reverts when users try to interact with PositionAction::repay.

Proof of Concept

Add the following in src/test/integration/IntegrationTestBase.sol, to create a balancer pool for the underlying token:

function _createBalancerUnderlyingTokenPool() internal returns (IComposableStablePool stablePool_) {
    deal(address(DAI), address(this), 5_000_000 * 1e18);
    deal(address(USDC), address(this), 5_000_000 * 1e6);
    deal(address(USDT), address(this), 5_000_000 * 1e6);
    underlyingToken.mint(address(this), 5_000_000 * 1e18);

    uint256[] memory maxAmountsIn = new uint256[](4);
    address[] memory assets = new address[](4);
    assets[0] = address(DAI);
    assets[1] = address(USDC);
    assets[2] = address(USDT);

    bool tokenPlaced;
    address tempAsset;
    for (uint256 i; i < assets.length; i++) {
        if (!tokenPlaced) {
            if (uint160(assets[i]) > uint160(address(underlyingToken))) {
                tokenPlaced = true;
                tempAsset = assets[i];
                assets[i] = address(underlyingToken);
            } else if (i == assets.length - 1) {
                assets[i] = address(underlyingToken);
            }
        } else {
            address placeholder = assets[i];
            assets[i] = tempAsset;
            tempAsset = placeholder;
        }
    }

    for (uint256 i; i < assets.length; i++) {
        maxAmountsIn[i] = ERC20(assets[i]).balanceOf(address(this));
        ERC20(assets[i]).safeApprove(address(balancerVault), maxAmountsIn[i]);
    }

    stablePool_ = stablePoolFactory.create(
        "Test Token Pool",
        "FUDT",
        assets,
        200,
        3e14, // swapFee (0.03%)
        address(this) // owner
    );

    balancerVault.joinPool(
        stablePool_.getPoolId(),
        address(this),
        address(this),
        JoinPoolRequest({
            assets: assets,
            maxAmountsIn: maxAmountsIn,
            userData: abi.encode(JoinKind.INIT, maxAmountsIn),
            fromInternalBalance: false
        })
    );
}

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

function test_wrongRepayAmount() public {
    uint256 depositAmount = 1_000 ether;
    uint256 borrowAmount = 500 ether;
    uint256 USDCamount = 100e6;

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

    bytes32[] memory poolIds = new bytes32[](1);
    poolIds[0] = _createBalancerUnderlyingTokenPool().getPoolId();

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

    vm.startPrank(user);

    // Approvals
    token.approve(address(vault), type(uint256).max);
    USDC.approve(address(userProxy), type(uint256).max);
    underlyingToken.approve(address(userProxy), type(uint256).max);

    // User deposits 1k ETH to vault
    vault.deposit(address(userProxy), depositAmount);

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

    // Collateral == 1k ETH
    // Debt == 500 ETH
    // User has 500 USDC
    (uint256 collateral, uint256 debt, , , , ) = vault.positions(address(userProxy));
    assertEq(collateral, depositAmount);
    assertEq(debt, borrowAmount);
    assertEq(USDC.balanceOf(user), USDCamount);

    // User repays his debt by swapping all his USDC to the underlying token
    userProxy.execute(
        address(positionAction),
        abi.encodeWithSelector(
            positionAction.repay.selector,
            address(userProxy),
            address(vault),
            CreditParams({
                amount: 0,
                creditor: user,
                auxSwap: SwapParams({
                    swapProtocol: SwapProtocol.BALANCER,
                    swapType: SwapType.EXACT_IN,
                    assetIn: address(USDC),
                    amount: USDCamount,
                    limit: 0,
                    recipient: address(userProxy),
                    deadline: block.timestamp,
                    args: abi.encode(poolIds, assets)
                })
            }),
            emptyPermitParams
        )
    );

    // Collateral == 1k ETH
    // Debt == 500 ETH
    // User has 0 USDC
    // User's USDC has been drained but no debt has been repaid
    (collateral, debt, , , , ) = vault.positions(address(userProxy));
    assertEq(collateral, depositAmount);
    assertEq(debt, borrowAmount);
    assertEq(USDC.balanceOf(user), 0);

    vm.stopPrank();
}

Tools Used

Manual review

Recommended Mitigation Steps

Update the moving amount of the underlying token after the swap, and use that value when repaying, which matches the logic in PositionAction::_deposit, something similar to the following:

function _repay(address vault, address position, CreditParams calldata creditParams, PermitParams calldata permitParams) internal {
    // transfer arbitrary token and swap to underlying token
    uint256 amount = creditParams.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),
                amount,
                permitParams
            );
        }
    }

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

Assessed type

Error

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as satisfactory

[0xAlix2]

@koolexcrypto - I believe this should be the primary issue for this issue group as it contains a more comprehensive explanation, a coded POC, and a mitigation suggestion.

[koolexcrypto]

#369 and this one #526 have both a good quality, will go with this one #526 .

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as selected for report