xiaoming90 - `msgValue` will not be populated if ETH is the secondary token

[github-actions[bot]]

xiaoming90

high

msgValue will not be populated if ETH is the secondary token

Summary

msgValue will not be populated if ETH is the secondary token in the two token leverage vault, leading to a loss of assets as the ETH is not forwarded to the Balancer Pool during a trade.

Vulnerability Detail

Based on the source code of the two token pool leverage vault, it is possible to deploy a vault to support a Balancer pool with an arbitrary token as the primary token and ETH as the secondary token. The primary token is always the borrowing currency in the vault.

However, Line 60 of TwoTokenPoolUtils._getPoolParams function below assumes that if one of the two tokens is ETH in the pool, it will always be the primary token or borrowing currency, which is not always the case. If the ETH is set as the secondary token, the msgValue will not be populated.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L45

File: TwoTokenPoolUtils.sol
44:     /// @notice Returns parameters for joining and exiting Balancer pools
45:     function _getPoolParams(
46:         TwoTokenPoolContext memory context,
47:         uint256 primaryAmount,
48:         uint256 secondaryAmount,
49:         bool isJoin
50:     ) internal pure returns (PoolParams memory) {
51:         IAsset[] memory assets = new IAsset[](2);
52:         assets[context.primaryIndex] = IAsset(context.primaryToken);
53:         assets[context.secondaryIndex] = IAsset(context.secondaryToken);
54: 
55:         uint256[] memory amounts = new uint256[](2);
56:         amounts[context.primaryIndex] = primaryAmount;
57:         amounts[context.secondaryIndex] = secondaryAmount;
58: 
59:         uint256 msgValue;
60:         if (isJoin && assets[context.primaryIndex] == IAsset(Deployments.ETH_ADDRESS)) {
61:             msgValue = amounts[context.primaryIndex];
62:         }
63: 
64:         return PoolParams(assets, amounts, msgValue);
65:     }

As a result, when the caller joins the Balancer pool, the params.msgValue will be empty, and no secondary token (ETH) will be forwarded to the Balancer pool. The ETH will remain stuck in the vault and the caller will receive much fewer BPT tokens in return.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/BalancerUtils.sol#L49

File: BalancerUtils.sol
48:     /// @notice Joins a balancer pool using exact tokens in
49:     function _joinPoolExactTokensIn(
50:         PoolContext memory context,
51:         PoolParams memory params,
52:         uint256 minBPT
53:     ) internal returns (uint256 bptAmount) {
54:         bptAmount = IERC20(address(context.pool)).balanceOf(address(this));
55:         Deployments.BALANCER_VAULT.joinPool{value: params.msgValue}(
56:             context.poolId,
57:             address(this),
58:             address(this),
59:             IBalancerVault.JoinPoolRequest(
60:                 params.assets,
61:                 params.amounts,
62:                 abi.encode(
63:                     IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,
64:                     params.amounts,
65:                     minBPT // Apply minBPT to prevent front running
66:                 ),
67:                 false // Don't use internal balances
68:             )
69:         );
70:         bptAmount =
71:             IERC20(address(context.pool)).balanceOf(address(this)) -
72:             bptAmount;
73:     }

Impact

Loss of assets for the callers as ETH will remain stuck in the vault and not forwarded to the Balancer Pool. Since the secondary token (ETH) is not forwarded to the Balancer pool, the caller will receive much fewer BPT tokens in return when joining the pool.

This issue affects the deposit and reinvest reward functions of the vault, which means that the depositor will receive fewer strategy tokens in return during depositing, and the vault will receive less BPT in return during reinvesting.

Code Snippet

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L45

Tool used

Manual Review

Recommendation

Consider populating the msgValue if the secondary token is ETH.

/// @notice Returns parameters for joining and exiting Balancer pools
function _getPoolParams(
    TwoTokenPoolContext memory context,
    uint256 primaryAmount,
    uint256 secondaryAmount,
    bool isJoin
) internal pure returns (PoolParams memory) {
    IAsset[] memory assets = new IAsset[](2);
    assets[context.primaryIndex] = IAsset(context.primaryToken);
    assets[context.secondaryIndex] = IAsset(context.secondaryToken);

    uint256[] memory amounts = new uint256[](2);
    amounts[context.primaryIndex] = primaryAmount;
    amounts[context.secondaryIndex] = secondaryAmount;

    uint256 msgValue;
    if (isJoin && assets[context.primaryIndex] == IAsset(Deployments.ETH_ADDRESS)) {
        msgValue = amounts[context.primaryIndex];
    }
+   if (isJoin && assets[context.secondaryIndex] == IAsset(Deployments.ETH_ADDRESS)) {
+       msgValue = amounts[context.secondaryIndex];
+   }

    return PoolParams(assets, amounts, msgValue);
}

[weitianjie2000]

valid, will fix