Compromised Third-Party Protocols Can Pull All Assets From Balancer Vaults

Summary

Compromised third-party protocols (Balancer and Aura) can pull all assets from the balancer vaults as they were given unlimited allowance.

Vulnerability Detail

Note: The root cause, PoC, and mitigation actions are the same for both MetaStable2TokenAuraVault and Boosted3TokenAuraVault vaults. Thus, the write-up of Boosted3TokenAuraVault vault is omitted for brevity.

During the vault initialization, it was observed that the vault gives Balancer maximum allowance to spend its primary (WETH) and secondary (stETH) tokens and gives Aura Finance maximum allowance to spend the BAL tokens. This effectively gives Balancer and Aura Finance the ability to pull all tokens (WETH, stETH, BAL) from the vaults.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/MetaStable2TokenAuraVault.sol#L44

File: MetaStable2TokenAuraVault.sol
44:     function initialize(InitParams calldata params)
45:         external
46:         initializer
47:         onlyNotionalOwner
48:     {
49:         __INIT_VAULT(params.name, params.borrowCurrencyId);
50:         BalancerVaultStorage.setStrategyVaultSettings(
51:             params.settings, MAX_ORACLE_QUERY_WINDOW, BalancerConstants.VAULT_PERCENT_BASIS
52:         );
53:         _twoTokenPoolContext()._approveBalancerTokens(address(_auraStakingContext().auraBooster));
54:     }

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

File: TwoTokenPoolUtils.sol
157:     function _approveBalancerTokens(TwoTokenPoolContext memory poolContext, address bptSpender) internal {
158:         IERC20(poolContext.primaryToken).checkApprove(address(Deployments.BALANCER_VAULT), type(uint256).max);
159:         IERC20(poolContext.secondaryToken).checkApprove(address(Deployments.BALANCER_VAULT), type(uint256).max);
160:         // Allow BPT spender to pull BALANCER_POOL_TOKEN
161:         IERC20(address(poolContext.basePool.pool)).checkApprove(bptSpender, type(uint256).max);
162:     }

Impact

If Balancer or Aura Finance is compromised, all the tokens (WETH, stETH, BAL) within the vaults can be stolen since the compromised contracts can pull all the tokens from the vault and transfer them to the attacker's address.

By doing so, the security of the Notional's leverage vault is heavily dependent on the Balancer or Aura Finance. The vault should adopt the principle of least privileges by only allowing Balancer and Aura Finance to pull/access the required amount of assets needed to carry out its task. No more or less.

Giving a blanket-wide allowance to Balancer or Aura Finance breaks this security principle. The vault should not place its entire trust on Balancer or Aura Finance and assume that it will not ever be hacked.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/MetaStable2TokenAuraVault.sol#L44 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L157

Tool used

Manual Review

Recommendation

Instead of giving third-party protocols (Balancer and Aura Finance) a maximum allowance to spend the assets on the vaults, consider granting the required amount of allowance when needed and revoking the allowance once it is no longer needed as shown below.

File: TwoTokenPoolUtils.sol
function _joinPoolAndStake(
    TwoTokenPoolContext memory poolContext,
    StrategyContext memory strategyContext,
    AuraStakingContext memory stakingContext,
    uint256 primaryAmount,
    uint256 secondaryAmount,
    uint256 minBPT
) internal returns (uint256 bptMinted) {
    // prettier-ignore
    PoolParams memory poolParams = poolContext._getPoolParams( 
        primaryAmount, 
        secondaryAmount,
        true // isJoin
    );
+   IERC20(poolContext.primaryToken).checkApprove(address(Deployments.BALANCER_VAULT), primaryAmount);
+   IERC20(poolContext.secondaryToken).checkApprove(address(Deployments.BALANCER_VAULT), secondaryAmount);
    bptMinted = BalancerUtils._joinPoolExactTokensIn({
        context: poolContext.basePool,
        params: poolParams,
        minBPT: minBPT
    });
+   IERC20(poolContext.primaryToken).checkRevoke(address(Deployments.BALANCER_VAULT));
+   IERC20(poolContext.secondaryToken).checkRevoke(address(Deployments.BALANCER_VAULT));

    // Check BPT threshold to make sure our share of the pool is
    // below maxBalancerPoolShare
    uint256 bptThreshold = strategyContext.vaultSettings._bptThreshold(
        poolContext.basePool.pool.totalSupply()
    );
    uint256 bptHeldAfterJoin = strategyContext.totalBPTHeld + bptMinted;
    if (bptHeldAfterJoin > bptThreshold)
        revert Errors.BalancerPoolShareTooHigh(bptHeldAfterJoin, bptThreshold);

    // Transfer token to Aura protocol for boosted staking
+   IERC20(address(poolContext.basePool.pool)).checkApprove(address(_auraStakingContext().auraBooster), bptMinted);
    stakingContext.auraBooster.deposit(stakingContext.auraPoolId, bptMinted, true); // stake = true
+   IERC20(address(poolContext.basePool.pool)).checkRevoke(address(_auraStakingContext().auraBooster));
}

This approach has been implemented within the TradingUtils._executeInternal function by the Notional team. Therefore, the same approach should be adopted here.

If the purpose of granting unlimited allowance to Balancer and Aura is to save gas by not having to call the approve function every single time during deposit and redemption, the risks of a security incident and its consequences seriously outweigh the benefits of a slight gas saving.

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Compromised Third-Party Protocols Can Pull All Assets From Balancer Vaults #58