ctf_sec - Balancer reentrancy check waste too much gas and can revert transaction in out of gas error

[sherlock-admin2]

ctf_sec

high

Balancer reentrancy check waste too much gas and can revert transaction in out of gas error

Summary

Balancer reentrancy check waste too much gas and can revert transactio in out of gas error

Vulnerability Detail

    function checkReentrancy(address balancerVault) external view {
        // solhint-disable max-line-length
        // https://github.com/balancer/balancer-v2-monorepo/blob/90f77293fef4b8782feae68643c745c754bac45c/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol
        (, bytes memory returnData) = balancerVault.staticcall(
            abi.encodeWithSelector(IVault.manageUserBalance.selector, new IVault.UserBalanceOp[](0))
        );
        if (keccak256(returnData) == REENTRANCY_ERROR_HASH) {
            revert BalancerVaultReentrancy();
        }
    }

Above is the reentrancy check for the balancer integration. The problem here is that this reentrancy check will not work.

According to balancer documentation the way to check for reentrancy in balancer is to call the ensureNotInVaultContext. Additionally this function can cause out of gas issues because of the static call. Static calls forward almost all gas when called, the best way to avoid an out of gas issue is to limit the gas consumption.

from balancer

balancer/balancer-v2-monorepo@7b2ab0a#diff-36f155e03e561d19a594fba949eb1929677863e769bd08861397f4c7396b0c71R50

 Staticcalls consume all gas forwarded to them on a revert caused by storage modification.
        // By default, almost the entire available gas is forwarded to the staticcall,
        // causing the entire call to revert with an 'out of gas' error.
		 We set the gas limit to 10k for the staticcall to
        // avoid wasting gas when it reverts due to storage modification.
        // `manageUserBalance` is a non-reentrant function in the Vault, so calling it invokes `_enterNonReentrant`
        // in the `ReentrancyGuard` contract, reproduced here:
		(, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }

Above shows the correct way to implement the static call to avoid out of gas issues. tokemak implementation should be updated to reflect this static call gas consumption fix.

Below is the function that should be called to avoid reentrancy with balancer integration and should not waste too much gas and revert the transaction

 * Call this at the top of any function that can cause a state change in a pool and is either public itself,
     * or called by a public function *outside* a Vault operation (e.g., join, exit, or swap).
     *
     * If this is *not* called in functions that are vulnerable to the read-only reentrancy issue described
     * here (https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345), those functions are unsafe,
     * and subject to manipulation that may result in loss of funds.
     */
    function ensureNotInVaultContext(IVault vault) internal {
        IVault.UserBalanceOp[] memory noop = new IVault.UserBalanceOp[](0);
        vault.manageUserBalance(noop);
    }
}

Impact

The reentrancy check does not work and addititonally the function may waste too much gas and result in an out of gas error and can block function call such as withdraw

Code Snippet

https://github.com/Tokemak/v2-core-audit-2023-07-14/blob/62445b8ee3365611534c96aef189642b721693bf/src/libs/BalancerUtilities.sol#L19-L28

Tool used

Manual Review

Recommendation

limit the static call gas consumption to avoid out of gas error

Duplicate of #822