PositionAction.sol#_deposit incorrectly checks auxSwap.assetIn should be equal to collateralParams.targetToken.

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

PositionAction.sol#_deposit incorrectly checks auxSwap.assetIn should be equal to collateralParams.targetToken. This is incorrect, because auxSwap.assetIn should be the token used to swap for collateralParams.targetToken.

Bug Description

First, let's inspect how deposit works with swap enabled:

1. collateralParams.collateralizer transfers auxSwap.assetIn token to Proxy.
2. Proxy performs a swap (Balancer or Uniswap) to get collateral token.
3. Deposit collateral tokens.

The issue here is, during step 2, the swap is to exchange auxSwap.assetIn for collateralParams.targetToken. This means that the two tokens must not be equal. However, the current implementation checks that they are the same. This means the swap feature is completely unusable.

    function _deposit(
        address vault,
        address position,
        CollateralParams calldata collateralParams,
        PermitParams calldata permitParams
    ) internal returns (uint256) {
        uint256 amount = collateralParams.amount;

        if (collateralParams.auxSwap.assetIn != address(0)) {
            if (
>               collateralParams.auxSwap.assetIn != collateralParams.targetToken ||
                collateralParams.auxSwap.recipient != address(this)
            ) revert PositionAction__deposit_InvalidAuxSwap();
            amount = _transferAndSwap(collateralParams.collateralizer, collateralParams.auxSwap, permitParams);
        } else if (collateralParams.collateralizer != address(this)) {
            _transferFrom(
                collateralParams.targetToken,
                collateralParams.collateralizer,
                address(this),
                amount,
                permitParams
            );
        }

        return _onDeposit(vault, position, collateralParams.targetToken, amount);
    }

Proof of Concept

N/A

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the check.

Assessed type

Invalid Validation

[amarcu]

The flow is correct, for example this is a test function where we go from USDC to the collateral token.

function test_deposit_vault_with_entry_swap_from_USDC() public {
        uint256 depositAmount = 10_000 * 1e6;
        uint256 amountOutMin = (depositAmount * 1e12 * 98) / 100; // convert 6 decimals to 18 and add 1% slippage

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

        // build increase collateral params
        bytes32[] memory poolIds = new bytes32[](1);
        poolIds[0] = stablePoolId;

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

        CollateralParams memory collateralParams = CollateralParams({
            targetToken: address(USDC),
            amount: 0, // not used for swaps
            collateralizer: address(user),
            auxSwap: SwapParams({
                swapProtocol: SwapProtocol.BALANCER,
                swapType: SwapType.EXACT_IN,
                assetIn: address(USDC),
                amount: depositAmount, // amount to swap in
                limit: amountOutMin, // min amount of collateral token to receive
                recipient: address(userProxy),
                deadline: block.timestamp + 100,
                args: abi.encode(poolIds, assets)
            })
        });

        uint256 expectedCollateral = _simulateBalancerSwap(collateralParams.auxSwap);

        vm.prank(user);
        USDC.approve(address(userProxy), depositAmount);

        vm.prank(user);
        userProxy.execute(
            address(positionAction),
            abi.encodeWithSelector(
                positionAction.deposit.selector,
                address(userProxy),
                address(vault),
                collateralParams,
                emptyPermitParams
            )
        );

        (uint256 collateral, uint256 debt, , , , ) = vault.positions(address(userProxy));

        assertEq(collateral, expectedCollateral);
        assertEq(debt, 0);
    }

Could we get POC on this issue?

[koolexcrypto]

Requesting a PoC from the warden , only in PJQA please.

will re-evaluate then.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[pkqs90]

@amarcu @koolexcrypto

First, let's see how CollateralParams is defined. From // optional swap from targetToken to collateral, or collateral to targetToken we can see that if there is a swap existent, the targetToken can be either the inputToken or the outputToken. The issue is that the buggy check forces targetToken to be inputToken, and does not allow for it being the output token.

struct CollateralParams {
    // token passed in or received by the caller
    address targetToken;
    // amount of collateral to add in CDPVault.tokenScale() or to remove in WAD
    uint256 amount;
    // address that will transfer the collateral or receive the collateral
    address collateralizer;
    // optional swap from `targetToken` to collateral, or collateral to `targetToken`
    SwapParams auxSwap;
}

        if (collateralParams.auxSwap.assetIn != address(0)) {
            if (
>               collateralParams.auxSwap.assetIn != collateralParams.targetToken ||
                collateralParams.auxSwap.recipient != address(this)
            ) revert PositionAction__deposit_InvalidAuxSwap();
            amount = _transferAndSwap(collateralParams.collateralizer, collateralParams.auxSwap, permitParams);
        } ...
        return _onDeposit(vault, position, collateralParams.targetToken, amount);

We can also see that at the end of the funcion, a _onDeposit(vault, position, collateralParams.targetToken, amount); is called to deposit the token to vault, which passes on the collateralParams.targetToken for depositing in vault.

The most common use case is to swap random input tokenA to targetToken, and deposit targetToken into vault. For this case, if we force targetToken to be inputToken, the swap doesn't make sense.

Now, responding to the passing unit test. The unit test dataflow is, user sets USDC as inputToken, and targetToken also as USDC. However, the vault receives a different token than USDC. But why did the unit test pass? Because for the PositionAction20.sol, the onDeposit() function doesn't about care the passed in token, thus it doesn't matter whichever token we set as targetToken. But for PositionAction4626.sol, the targetToken is used to check if it is the collateral for vault, and if not, it will perform a ERC4626 deposit first, and in this case, if the targetToken is incorrect, the deposit would fail.

        vault = createCDPVault(
            token, // token
            5_000_000 ether, // debt ceiling
            0, // debt floor
            1.25 ether, // liquidation ratio
            1.0 ether, // liquidation penalty
            1.05 ether // liquidation discount
        );

PositionAction20.sol

    function _onDeposit(address vault, address position, address /*src*/, uint256 amount) internal override returns (uint256) {
        address collateralToken = address(ICDPVault(vault).token());
        IERC20(collateralToken).forceApprove(vault, amount);
        return ICDPVault(vault).deposit(position, amount);
    }

    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);
        return ICDPVault(vault).deposit(address(this), amount);
    }

[koolexcrypto]

@pkqs90 PoC (coded) is requested. Please provide it ASAP.

Also the impact is not clear. I am assuming the report implies that , the functionality doesn't work as intended. But elaboration on this is required.

PositionAction.sol#_deposit incorrectly checks auxSwap.assetIn should be equal to collateralParams.targetToken. This is incorrect, because auxSwap.assetIn should be the token used to swap for collateralParams.targetToken.

[pkqs90]

@koolexcrypto

I created a PoC based on the UT the sponsors provided. There are only 2 changes (which I also commented out in code):

1. Use PositionAction4626 instead of PositionAction20.
2. Change targetToken to token, since the vault's collateral token is token. (The previous UT test_deposit_vault_with_entry_swap_from_USDC marked this as USDC, which is incorrect).

Put this code inside PositionAction20.t.sol, and you will find this code reverts with error PositionAction__deposit_InvalidAuxSwap. However, this should not revert, because the input it provides is correct.

The use case is: User initially has USDC, user wishes to perform swap from USDC to token and deposit token in vault.

    function test_PoC() public {
        // Change 1: Use PositionAction4626 instead of PositionAction20.
        PositionAction4626 positionAction4626 = new PositionAction4626(
            address(flashlender),
            address(swapAction),
            address(poolAction),
            address(vaultRegistry)
        );

        uint256 depositAmount = 10_000 * 1e6;
        uint256 amountOutMin = (depositAmount * 1e12 * 98) / 100; // convert 6 decimals to 18 and add 1% slippage

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

        // build increase collateral params
        bytes32[] memory poolIds = new bytes32[](1);
        poolIds[0] = stablePoolId;

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

        // Change 2: Change targetToken to `token`, since the vault's collateral token is `token`. (The previous UT `test_deposit_vault_with_entry_swap_from_USDC` marked this as `USDC`, which is incorrect ).
        CollateralParams memory collateralParams = CollateralParams({
            targetToken: address(token),
            amount: 0, // not used for swaps
            collateralizer: address(user),
            auxSwap: SwapParams({
                swapProtocol: SwapProtocol.BALANCER,
                swapType: SwapType.EXACT_IN,
                assetIn: address(USDC),
                amount: depositAmount, // amount to swap in
                limit: amountOutMin, // min amount of collateral token to receive
                recipient: address(userProxy),
                deadline: block.timestamp + 100,
                args: abi.encode(poolIds, assets)
            })
        });

        uint256 expectedCollateral = _simulateBalancerSwap(collateralParams.auxSwap);

        vm.prank(user);
        USDC.approve(address(userProxy), depositAmount);

        vm.prank(user);
        userProxy.execute(
            address(positionAction4626),
            abi.encodeWithSelector(
                positionAction4626.deposit.selector,
                address(userProxy),
                address(vault),
                collateralParams,
                emptyPermitParams
            )
        );

        (uint256 collateral, uint256 debt, , , , ) = vault.positions(address(userProxy));

        assertEq(collateral, expectedCollateral);
        assertEq(debt, 0);
    }

[koolexcrypto]

@pkqs90

Error (7920): Identifier not found or not unique.
   --> src/test/integration/PositionAction20.t.sol:861:9:
    |
861 |         PositionAction4626 positionAction4626 = new PositionAction4626(

Anything to add here to fix this error?

[pkqs90]

@koolexcrypto Add this import in the beginning of file:

import {PositionAction4626} from "../../proxy/PositionAction4626.sol";

[koolexcrypto]

The function reverts as @pkqs90 stated.

[FAIL. Reason: PositionAction__deposit_InvalidAuxSwap()] test_PoC() (gas: 4270979)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 884.18ms (4.20ms CPU time)

Ran 1 test suite in 887.08ms (884.18ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/test/integration/PositionAction20.t.sol:PositionAction20Test
[FAIL. Reason: PositionAction__deposit_InvalidAuxSwap()] test_PoC() (gas: 4270979)

---

The most common use case is to swap random input tokenA to targetToken, and deposit targetToken into vault. For this case, if we force targetToken to be inputToken, the swap doesn't make sense.

I believe this is a valid concern.

@amarcu please provide your feedback on this.

[koolexcrypto]

Stays as is. The function doesn't seem to work as intended.