OOG / unexpected reverts due to incorrect usage of staticcall.
The function `checkReentrancy` in `BalancerUtilities.sol` is used to check if the balancer contract has been re-entered or not. It does this by doing a `staticcall` on the pool contract and checking the return value. According to the solidity docs, if a `staticcall` encounters a state change, it burns up all gas and returns. The `checkReentrancy` tries to call `manageUserBalance` on the vault contract, and returns if it finds a state change.\\nThe issue is that this burns up all the gas sent with the call. According to EIP150, a call gets allocated 63/64 bits of the gas, and the entire 63/64 parts of the gas is burnt up after the staticcall, since the staticcall will always encounter a storage change. This is also highlighted in the balancer monorepo, which has guidelines on how to check re-entrancy here.\\nThis can also be shown with a simple POC.\\n```\\nunction testAttack() public {\\n        mockRootPrice(WSTETH, 1_123_300_000_000_000_000); //wstETH\\n        mockRootPrice(CBETH, 1_034_300_000_000_000_000); //cbETH\\n\\n        IBalancerMetaStablePool pool = IBalancerMetaStablePool(WSTETH_CBETH_POOL);\\n\\n        address[] memory assets = new address[](2);\\n        assets[0] = WSTETH;\\n        assets[1] = CBETH;\\n        uint256[] memory amounts = new uint256[](2);\\n        amounts[0] = 10_000 ether;\\n        amounts[1] = 0;\\n\\n        IBalancerVault.JoinPoolRequest memory joinRequest = IBalancerVault.JoinPoolRequest({\\n            assets: assets,\\n            maxAmountsIn: amounts, // maxAmountsIn,\\n            userData: abi.encode(\\n                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,\\n                amounts, //maxAmountsIn,\\n                0\\n            ),\\n            fromInternalBalance: false\\n        });\\n\\n        IBalancerVault.SingleSwap memory swapRequest = IBalancerVault.SingleSwap({\\n            poolId: 0x9c6d47ff73e0f5e51be5fd53236e3f595c5793f200020000000000000000042c,\\n            kind: IBalancerVault.SwapKind.GIVEN_IN,\\n            assetIn: WSTETH,\\n            assetOut: CBETH,\\n            amount: amounts[0],\\n            userData: abi.encode(\\n                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,\\n                amounts, //maxAmountsIn,\\n                0\\n            )\\n        });\\n\\n        IBalancerVault.FundManagement memory funds = IBalancerVault.FundManagement({\\n            sender: address(this),\\n            fromInternalBalance: false,\\n            recipient: payable(address(this)),\\n            toInternalBalance: false\\n        });\\n\\n        emit log_named_uint("Gas before price1", gasleft());\\n        uint256 price1 = oracle.getPriceInEth(WSTETH_CBETH_POOL);\\n        emit log_named_uint("price1", price1);\\n        emit log_named_uint("Gas after price1 ", gasleft());\\n    }\\n```\\n\\nThe oracle is called to get a price. This oracle calls the `checkReentrancy` function and burns up the gas. The gas left is checked before and after this call.\\nThe output shows this:\\n```\\n[PASS] testAttack() (gas: 9203730962297323943)\\nLogs:\\nGas before price1: 9223372036854745204\\nprice1: 1006294352158612428\\nGas after price1 : 425625349158468958\\n```\\n\\nThis shows that 96% of the gas sent is burnt up in the oracle call.
According to the monorepo here, the staticall must be allocated a fixed amount of gas. Change the reentrancy check to the following.\\n```\\n(, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }(\\n            abi.encodeWithSelector(vault.manageUserBalance.selector, 0)\\n        );\\n```\\n\\nThis ensures gas isn't burnt up without reason.
This causes the contract to burn up 63/64 bits of gas in a single check. If there are lots of operations after this call, the call can revert due to running out of gas. This can lead to a DOS of the contract.\\nCurrent opinion is to reject escalation and keep issue medium severity.\\nJeffCX\\nPutting a limit on the gas is not a task for the protocol\\nsir, please read the report again, the flawed logic in the code charge user 100x gas in every transaction in every withdrawal\\nin a single transaction, the cost burnt can by minimal\\nidk how do state it more clearly, emm if you put money in the bank, you expect to pay 1 USD for withdrawal transaction fee, but every time you have to pay 100 USD withdrawal fee because of the bug\\nthis cause loss of fund for every user in every transaction for not only you but every user...\\nEvert0x\\n@JeffCX what are the exact numbers on the withdrawal costs? E.g. if I want to withdraw $10k, how much gas can I expect to pay? If this is a significant amount I can see the argument for\\nHow to identify a high issue: Definite loss of funds without limiting external conditions.\\nBut it's not clear how much this will be assuming current mainnet conditions.\\nJeffCX\\nI write a simpe POC\\n```\\n// SPDX-License-Identifier: UNLICENSED\\npragma solidity ^0.8.13;\\n\\nimport "forge-std/Test.sol";\\nimport "forge-std/console.sol";\\n\\nimport "@openzeppelin/contracts/token/ERC20/ERC20.sol";\\n\\ncontract MockERC20 is ERC20 {\\n    constructor()ERC20("MyToken", "MTK")\\n    {}\\n\\n    function mint(address to, uint256 amount) public {\\n        _mint(to, amount);\\n    }\\n}\\n\\ninterface ICheckRetrancy {\\n    function checkRentrancy() external;\\n}\\n\\ncontract RentrancyCheck {\\n\\n\\n    uint256 state = 10;\\n\\n    function checkRentrancy() external {\\n        address(this).staticcall(abi.encodeWithSignature("hihi()"));\\n    }\\n\\n    function hihi() public {\\n        state = 11;\\n    }\\n\\n}\\n\\ncontract Vault {\\n\\n    address balancerAddr;\\n    bool checkRentrancy;\\n\\n    constructor(bool _checkRentrancy, address _balancerAddr) {\\n        checkRentrancy = _checkRentrancy;\\n        balancerAddr = _balancerAddr;\\n    }\\n\\n    function toggleCheck(bool _state) public {\\n        checkRentrancy = _state;\\n    }\\n\\n    function withdraw(address token, uint256 amount) public {\\n\\n        if(checkRentrancy) {\\n            ICheckRetrancy(balancerAddr).checkRentrancy();\\n        }\\n\\n        IERC20(token).transfer(msg.sender, amount);\\n    \\n    }\\n\\n}\\n\\n\\ncontract CounterTest is Test {\\n\\n    using stdStorage for StdStorage;\\n    StdStorage stdlib;\\n\\n    MockERC20 token;\\n    Vault vault;\\n    RentrancyCheck rentrancyCheck;\\n\\n    address user = vm.addr(5201314);\\n\\n    function setUp() public {\\n    \\n        token = new MockERC20();\\n        rentrancyCheck = new RentrancyCheck();\\n        vault = new Vault(false, address(rentrancyCheck));\\n        token.mint(address(vault), 100000000 ether);\\n\\n        vm.deal(user, 100 ether);\\n    \\n        // vault.toggleCheck(true);\\n\\n    }\\n\\n    function testPOC() public {\\n\\n        uint256 gas = gasleft();\\n        uint256 amount = 100 ether;\\n        vault.withdraw(address(token), amount);\\n        console.log(gas - gasleft());\\n\\n    }\\n\\n}\\n```\\n\\nthe call is\\n```\\nif check reentrancy flag is true\\n\\nuser withdraw -> \\ncheck reentrancy staticall revert and consume most of the gas \\n-> withdraw completed\\n```\\n\\nor\\n```\\nif check reentrancy flag is false\\n\\nuser withdraw ->\\n-> withdraw completed\\n```\\n\\nnote first we do not check the reentrancy\\n```\\n// vault.toggleCheck(true);\\n```\\n\\nwe run\\nthe gas cost is 42335\\n```\\nRunning 1 test for test/Counter.t.sol:CounterTest\\n[PASS] testPOC() (gas: 45438)\\nLogs:\\n  42335\\n```\\n\\nthen we uncomment the vault.toggleCheck(true) and check the reentrancy that revert in staticcall\\n```\\nvault.toggleCheck(true);\\n```\\n\\nwe run the same test again, this is the output, as we can see the gas cost surge\\n```\\nRunning 1 test for test/Counter.t.sol:CounterTest\\n[PASS] testPOC() (gas: 9554791)\\nLogs:\\n  9551688\\n```\\n\\nthen we can use this python scirpt to estimate how much gas is overpaid as lost of fund\\n```\\nregular =  42313\\n\\noverpaid = 9551666\\n\\n\\ncost = 0.000000045 * (overpaid - regular);\\n\\nprint(cost)\\n```\\n\\nthe cost is\\n```\\n0.427920885 ETH\\n```\\n\\nin a single withdraw, assume user lost 0.427 ETH,\\nif 500 user withdraw 20 times each and the total number of transaction is 10000\\nthe lose on gas is 10000 * 0.427 ETH\\nJeffCX\\nnote that the more gas limit user set, the more fund user lose in gas\\nbut we are interested in what the lowest amount of gas limit user that user can set the pay for withdrawal transaction\\nI did some fuzzing\\nthat number is 1800000 unit of gas\\nthe command to run the test is\\nsetting gas limit lower than 1800000 unit of gas is likely to revert in out of gas\\nunder this setting, the overpaid transaction cost is 1730089\\n```\\nRunning 1 test for test/Counter.t.sol:CounterTest\\n[PASS] testPOC() (gas: 1733192)\\nLogs:\\n  1730089\\n```\\n\\nin other words,\\nin each withdrawal for every user, user can lose 0.073 ETH, (1730089 uint of gas * 45 gwei -> 0.000000045 ETH)\\nassume there are 1000 user, each withdraw 10 times, they make 1000 * 10 = 100_00 transaction\\nso the total lost is 100_00 * 0.07 = 700 ETH\\nin reality the gas is more than that because user may use more than 1800000 unit of gas to finalize the withdrawal transaction\\nEvert0x\\n@JeffCX thanks for putting in the effort to make this estimation.\\nBut as far as I can see, your estimation doesn't use the actual contracts in scope. But maybe that's irrelevant to make your point.\\nThis seems like the key sentence\\nin each withdrawal for every user, user can lose 0.073 ETH,\\nThis is an extra $100-$150 dollars per withdrawal action.\\nThis is not a very significant amount in my opinion. I assume an optimized withdrawal transaction will cost between $20-$50. So the difference is not as big.\\nJeffCX\\nSir, I don't think the method A and method B example applies in the codebase and in this issue\\nthere is only one method for user to withdraw share from the vault\\nI can add more detail to explain how this impact withdraw using top-down approach\\nUser can withdraw by calling withdraw in LMPVault.sol and triggers _withdraw\\nthe _withdraw calls the method _calcUserWithdrawSharesToBurn\\nthis calls LMPDebt._calcUserWithdrawSharesToBurn\\nwe need to know the debt value by calling destVault.debtValue\\nthis calls this line of code\\nthis calls the oracle code\\n`uint256 price = _systemRegistry.rootPriceOracle().getPriceInEth(_underlying);`\\nthen if the dest vault is the balancer vault, balancer reetrancy check is triggered to waste 63 / 64 waste in oracle code\\nso there is no function A and function B call\\nas long as user can withdraw and wants to withdraw share from balancer vault, 100x gas overpayment is required\\nthe POC is a simplified flow of this\\nit is ok to disagree sir:)\\nEvert0x\\nResult: Medium Has Duplicates\\nsherlock-admin2\\nEscalations have been resolved successfully!\\nEscalation status:\\nJEFFCX: rejected
```\\nunction testAttack() public {\\n        mockRootPrice(WSTETH, 1_123_300_000_000_000_000); //wstETH\\n        mockRootPrice(CBETH, 1_034_300_000_000_000_000); //cbETH\\n\\n        IBalancerMetaStablePool pool = IBalancerMetaStablePool(WSTETH_CBETH_POOL);\\n\\n        address[] memory assets = new address[](2);\\n        assets[0] = WSTETH;\\n        assets[1] = CBETH;\\n        uint256[] memory amounts = new uint256[](2);\\n        amounts[0] = 10_000 ether;\\n        amounts[1] = 0;\\n\\n        IBalancerVault.JoinPoolRequest memory joinRequest = IBalancerVault.JoinPoolRequest({\\n            assets: assets,\\n            maxAmountsIn: amounts, // maxAmountsIn,\\n            userData: abi.encode(\\n                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,\\n                amounts, //maxAmountsIn,\\n                0\\n            ),\\n            fromInternalBalance: false\\n        });\\n\\n        IBalancerVault.SingleSwap memory swapRequest = IBalancerVault.SingleSwap({\\n            poolId: 0x9c6d47ff73e0f5e51be5fd53236e3f595c5793f200020000000000000000042c,\\n            kind: IBalancerVault.SwapKind.GIVEN_IN,\\n            assetIn: WSTETH,\\n            assetOut: CBETH,\\n            amount: amounts[0],\\n            userData: abi.encode(\\n                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,\\n                amounts, //maxAmountsIn,\\n                0\\n            )\\n        });\\n\\n        IBalancerVault.FundManagement memory funds = IBalancerVault.FundManagement({\\n            sender: address(this),\\n            fromInternalBalance: false,\\n            recipient: payable(address(this)),\\n            toInternalBalance: false\\n        });\\n\\n        emit log_named_uint("Gas before price1", gasleft());\\n        uint256 price1 = oracle.getPriceInEth(WSTETH_CBETH_POOL);\\n        emit log_named_uint("price1", price1);\\n        emit log_named_uint("Gas after price1 ", gasleft());\\n    }\\n```\\n