Improper Handling of msg.value in ETH Pool Join Operations

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PoolAction.sol#L142

Vulnerability details

Description

The identified vulnerability pertains to the improper handling of msg.value within the join() function, particularly when users attempt to join a Balancer pool using ETH. This flaw arises from the function’s failure to correctly manage the msg.value parameter, which represents the amount of ETH sent by the user. As a result, the ETH provided by the user may be sent to the contract or Vault but remains unwrapped and unutilized in the intended pool join operation, leading to substantial financial loss and user dissatisfaction.

The Balancer protocol supports users joining liquidity pools using various tokens, including ETH. Typically, when a user sends ETH, it is wrapped into WETH via the balancerVault.joinPool() function and subsequently added to the pool. This process hinges on the correct forwarding of the msg.value parameter from the join() function to the balancerVault.joinPool() function.

However, the vulnerability manifests when the join() function neglects to properly pass the msg.value to the balancerVault.joinPool() function. In such instances, while the ETH is received by the contract or Vault, it is neither wrapped into WETH nor added to the pool. This erroneous handling can lead to two primary issues:

• Loss of Funds: The ETH provided by the user becomes trapped within the contract or Vault, as it is not employed in the pool join operation. Consequently, the user does not receive the corresponding pool shares, effectively losing the ETH they intended to use.

Proof of Concept

• Scenario: Alice intends to join a Balancer pool by sending 50 ETH. She calls the join() function, anticipating that her ETH will be wrapped into WETH and used to join the pool.

• Flawed Execution: Alice’s 50 ETH is correctly sent as msg.value to the join() function. However, due to an improper implementation, the msg.value is not passed to the balancerVault.joinPool() function as required.

• Outcome: The ETH is received by the contract or Vault but remains unwrapped and unutilized in the pool join operation. Consequently, the joinPool function does not recognize any ETH being sent, and no pool shares are issued to Alice. Her 50 ETH is effectively trapped within the contract or Vault, resulting in a total loss of funds.

function _balancerJoin(PoolActionParams memory poolActionParams) internal {
    (bytes32 poolId, address[] memory assets, uint256[] memory assetsIn, uint256[] memory maxAmountsIn) = abi.decode(
        poolActionParams.args, (bytes32, address[], uint256[], uint256[])
    );
    for (uint256 i = 0; i < assets.length; ) {
        if (maxAmountsIn[i] != 0) {
            IERC20(assets[i]).forceApprove(address(balancerVault), maxAmountsIn[i]);
        }
        unchecked {
            ++i;
        }
    }
    balancerVault.joinPool(     <-------
        poolId,
        address(this),
        poolActionParams.recipient,
        IBalancerVault.JoinPoolRequest({
            assets: assets,
            maxAmountsIn: maxAmountsIn,
            userData: userData,
            fromInternalBalance: false
        })
    );
}

The key issue lies in the absence of an explicit forwarding of msg.value to the balancerVault.joinPool() function, which is crucial for the proper handling of ETH in pool join operations.

Impact

The impact of this vulnerability is considerable. Users who attempt to join pools using ETH risk losing their funds due to the improper handling of msg.value. The ETH provided by users is not converted into WETH or added to the liquidity pool, resulting in the loss of both the ETH and the expected pool shares. This scenario not only leads to financial loss but also damages user trust in the protocol, as the transaction behavior deviates from expected outcomes.

Tools Used

Manual Review, related issue - https://solodit.xyz/issues/h-6-msgvalue-will-not-be-populated-if-eth-is-the-secondary-token-sherlock-notional-notional-update-git

Recommended Mitigation Steps

To mitigate this vulnerability, it is imperative to ensure that the join() function correctly forwards msg.value to the balancerVault.joinPool() function whenever ETH is sent. This can be achieved by explicitly including the msg.value in the joinPool call, as demonstrated in the following code example:

balancerVault.joinPool{value: msg.value}(
    poolId,
    address(this),
    poolActionParams.recipient,
    IBalancerVault.JoinPoolRequest({
        assets: assets,
        maxAmountsIn: maxAmountsIn,
        userData: abi.encode(JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT, assetsIn, poolActionParams.minOut),
        fromInternalBalance: false
    })
);

Assessed type

Payable

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[c4-judge]

koolexcrypto marked the issue as duplicate of #70

[c4-judge]

koolexcrypto marked the issue as satisfactory