PoolAction::updateLeverJoin wrongly updates assetsIn array, leading to PositionAction4626::_onIncreaseLever to always revert

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/proxy/PoolAction.sol#L181-L223

Vulnerability details

Impact

Users can use PositionAction4626 to interact with the corresponding vault using their opened positions. PositionAction4626 allows users to deposit/withdraw/leverage their positions, increaseLever enables users to increase their positions' collateral and debt, for the most part, it's the same as for PositionAction20. Where users swap the lent borrow tokens for collateral tokens, and then deposit them into the position.

The only change is that PositionAction4626 allows users on top of that to join a Balancer pool with the swapped tokens. This is done in PositionAction4626::_onIncreaseLever.
The main part that we care about is PoolAction::updateLeverJoin which adds the upfront amount to the amounts in. The way Balancer works is that it accepts an array of "amounts in", according to the tokens array where indices should match, BUT it should skip the BPT token, this is where the function messes up.
This is mainly done in the following loop:

for (uint256 i = 0; i < len; ) {
    uint256 assetIndex = i - (skipIndex ? 1 : 0);
    if (assets[i] == joinToken) {
        maxAmountsIn[i] = joinAmount;
        assetsIn[assetIndex] = joinAmount;
    } else if (assets[i] == upFrontToken && assets[i] != poolToken) {
        maxAmountsIn[i] = upfrontAmount;
        assetsIn[assetIndex] = upfrontAmount;
    } else {
        skipIndex = skipIndex || assets[i] == poolToken;
    }
    unchecked {
        i++;
    }
}

The goal of the above loop is to add the upfront amount to the corresponding amountIn. The protocol passes the poolToken as the collaterals 4626's underlying token, which is not always true, and in most cases, it won't match any of the tokens array. Because of this, the above for loop will be wrongly updating and overriding the assetsIn array.

This blocks users from increasing the leverage of their positions where the collateral is an ERC4626 token.

Proof of Concept

In the below POC, we pass the following:

tokensIn = [49 ether, 0]

but because of what's mentioned above and the wrong "skipping" logic, the tokensIn comes out as:

tokensIn = [49 ether, 49 ether]

contract PositionAction4626_Lever_Test is IntegrationTestBase {
    using SafeERC20 for ERC20;

    PRBProxy userProxy;
    address user;
    CDPVault vault;
    StakingLPEth stakingLPEth;
    PositionAction4626 positionAction;
    PermitParams emptyPermitParams;
    SwapParams emptySwap;
    PoolActionParams emptyPoolActionParams;

    bytes32[] weightedPoolIdArray;

    address constant wstETH_bb_a_WETH_BPTl = 0x41503C9D499ddbd1dCdf818a1b05e9774203Bf46;
    address constant wstETH = 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0;
    address constant bbaweth = 0xbB6881874825E60e1160416D6C426eae65f2459E;
    bytes32 constant poolId = 0x41503c9d499ddbd1dcdf818a1b05e9774203bf46000000000000000000000594;

    function setUp() public override {
        super.setUp();
        setGlobalDebtCeiling(15_000_000 ether);

        token = ERC20PresetMinterPauser(wstETH);

        stakingLPEth = new StakingLPEth(address(token), "Staking LP ETH", "sLPETH");
        vault = createCDPVault(stakingLPEth, 5_000_000 ether, 0, 1.25 ether, 1.0 ether, 1.05 ether);
        createGaugeAndSetGauge(address(vault), address(stakingLPEth));

        user = vm.addr(0x12341234);
        userProxy = PRBProxy(payable(address(prbProxyRegistry.deployFor(user))));

        positionAction = new PositionAction4626(
            address(flashlender),
            address(swapAction),
            address(poolAction),
            address(vaultRegistry)
        );

        weightedUnderlierPoolId = _createBalancerPool(address(token), address(underlyingToken)).getPoolId();

        oracle.updateSpot(address(token), 1 ether);
        oracle.updateSpot(address(stakingLPEth), 1 ether);
        weightedPoolIdArray.push(weightedUnderlierPoolId);
    }

    function test_updateLeverJoin_increaseLeverageDOS() public {
        uint256 depositAmount = 200 ether;
        uint256 borrowAmount = 100 ether;

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

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

        vm.startPrank(user);

        token.approve(address(stakingLPEth), depositAmount);
        stakingLPEth.approve(address(userProxy), depositAmount);

        // Deposit `wstETH` to get `sLPETH`
        stakingLPEth.deposit(depositAmount, user);

        // Deposit `sLPETH` to vault
        userProxy.execute(
            address(positionAction),
            abi.encodeWithSelector(
                positionAction.deposit.selector,
                address(userProxy),
                address(vault),
                CollateralParams({
                    targetToken: address(stakingLPEth),
                    amount: depositAmount,
                    collateralizer: address(user),
                    auxSwap: emptySwap
                }),
                emptyPermitParams
            )
        );

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

        address[] memory tokens = new address[](3);
        tokens[0] = wstETH_bb_a_WETH_BPTl;
        tokens[1] = wstETH;
        tokens[2] = bbaweth;

        uint256[] memory maxAmountsIn = new uint256[](3);
        maxAmountsIn[0] = 0;
        maxAmountsIn[1] = borrowAmount / 2 - 1 ether;
        maxAmountsIn[2] = 0;

        uint256[] memory tokensIn = new uint256[](2);
        tokensIn[0] = borrowAmount / 2 - 1 ether;
        tokensIn[1] = 0;

        // Increase the leverage of the position, reverts
        // Swapping underlying tokens to collateral tokens, the joining a Balancer pool
        vm.expectRevert(bytes("BAL#506"));
        userProxy.execute(
            address(positionAction),
            abi.encodeWithSelector(
                positionAction.increaseLever.selector,
                LeverParams({
                    position: address(userProxy),
                    vault: address(vault),
                    collateralToken: address(stakingLPEth),
                    primarySwap: SwapParams({
                        swapProtocol: SwapProtocol.BALANCER,
                        swapType: SwapType.EXACT_IN,
                        assetIn: address(underlyingToken),
                        amount: borrowAmount / 2,
                        limit: 0,
                        recipient: address(positionAction),
                        deadline: block.timestamp,
                        args: abi.encode(weightedPoolIdArray, assets)
                    }),
                    auxSwap: emptySwap,
                    auxAction: PoolActionParams(
                        Protocol.BALANCER,
                        0,
                        user,
                        abi.encode(poolId, tokens, tokensIn, maxAmountsIn)
                    )
                }),
                address(0),
                0,
                address(user),
                emptyPermitParams
            )
        );
    }

    function _createBalancerPool(address t1, address t2) internal returns (IComposableStablePool pool_) {
        uint256 amount = 5_000_000_000 ether;
        deal(t1, address(this), amount);
        deal(t2, address(this), amount);

        uint256[] memory maxAmountsIn = new uint256[](2);
        address[] memory assets = new address[](2);
        assets[0] = t1;
        uint256[] memory weights = new uint256[](2);
        weights[0] = 500000000000000000;
        weights[1] = 500000000000000000;

        bool tokenPlaced;
        address tempAsset;
        for (uint256 i; i < assets.length; i++) {
            if (!tokenPlaced) {
                if (uint160(assets[i]) > uint160(t2)) {
                    tokenPlaced = true;
                    tempAsset = assets[i];
                    assets[i] = t2;
                } else if (i == assets.length - 1) {
                    assets[i] = t2;
                }
            } 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]);
        }

        pool_ = weightedPoolFactory.create(
            "50WETH-50TOKEN",
            "50WETH-50TOKEN",
            assets,
            weights,
            3e14, // swapFee (0.03%)
            address(this) // owner
        );

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

    function getForkBlockNumber() internal pure virtual override(IntegrationTestBase) returns (uint256) {
        return 17870449; // Aug-08-2023 01:17:35 PM +UTC
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Set the poolToken according to the Balancer's vault and PoolId, something to:

function updateLeverJoin(
    PoolActionParams memory poolActionParams,
    address joinToken,
    address upFrontToken,
    uint256 flashLoanAmount,
    uint256 upfrontAmount
) external view returns (PoolActionParams memory outParams) {
    outParams = poolActionParams;

    if (poolActionParams.protocol == Protocol.BALANCER) {
        (bytes32 poolId, address[] memory assets, uint256[] memory assetsIn, uint256[] memory maxAmountsIn) = abi
            .decode(poolActionParams.args, (bytes32, address[], uint256[], uint256[]));

        address poolToken = balancerVault.getPool(poolId);

        uint256 len = assets.length;
        // the offset is needed because of the BPT token that needs to be skipped from the join
        bool skipIndex = false;
        uint256 joinAmount = flashLoanAmount;
        if (upFrontToken == joinToken) {
            joinAmount += upfrontAmount;
        }

        // update the join parameters with the new amounts
        for (uint256 i = 0; i < len; ) {
            uint256 assetIndex = i - (skipIndex ? 1 : 0);
            if (assets[i] == joinToken) {
                maxAmountsIn[i] = joinAmount;
                assetsIn[assetIndex] = joinAmount;
            } else if (assets[i] == upFrontToken && assets[i] != poolToken) {
                maxAmountsIn[i] = upfrontAmount;
                assetsIn[assetIndex] = upfrontAmount;
            } else {
                skipIndex = skipIndex || assets[i] == poolToken;
            }
            unchecked {
                i++;
            }
        }

        // update the join parameters
        outParams.args = abi.encode(poolId, assets, assetsIn, maxAmountsIn);
    }
}

Assessed type

DoS

[c4-sponsor]

@amarcu Sponsors are not allowed to close, reopen, or assign issues or pull requests.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report