0xMR0 - Inadequate Gas Limit in Staticcall while checking for balancer reentrancy would fail

[sherlock-admin]

0xMR0

medium

Inadequate Gas Limit in Staticcall while checking for balancer reentrancy would fail

Summary

Inadequate Gas Limit in Staticcall while checking for balancer reentrancy would fail

Vulnerability Detail

VaultReentrancyLib.ensureNotInVaultContext() 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.

ensureNotInVaultContext() is given as below,

    function ensureNotInVaultContext(IVault vault) internal view {

         . . . some comments

        // Reduced to 1k gas to avoid large gas cost predictions when using this function in a non-view function
>>      (, bytes memory revertData) = address(vault).staticcall{gas: 1_000}(
            abi.encodeWithSelector(vault.manageUserBalance.selector, 0)
        );

        _require(revertData.length == 0, Errors.REENTRANCY);
    }

The gas limit for the static call is set to 1_000, which is insufficient for the target function i.e vault.manageUserBalance() to execute successfully if it consumes more gas than the specified limit.

Balancer VaultReentrancyLib.sol has provided a gas limit of 10_000 for staticcall so that the function would not fail. This can be checked here

    function ensureNotInVaultContext(IVault vault) internal view {
        . . . some comments

>>        (, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }(
            abi.encodeWithSelector(vault.manageUserBalance.selector, 0)
        );

        _require(revertData.length == 0, Errors.REENTRANCY);
    }

For successfully working of staticcall, a 10_000 gas limit is enough.

Referencing this documentation,

staticcall gas is calculated as,

AA-4: STATICCALL Gas Calculation: base_gas = access_cost + mem_expansion_cost Calculate the gas_sent_with_call below.

And the final cost of the operation: gas_cost = base_gas + gas_sent_with_call

EIP-150 increased the base_cost of the CALL opcode from 40 to 700 gas, but most contracts in use at the time were sending available_gas - 40 with every call. So, when base_cost increased, these contracts were suddenly trying to send more gas than they had left (requested_gas > remaining_gas). To avoid breaking these contracts, if the requested_gas is more than remaining_gas, we send all_but_one_64th of remaining_gas instead of trying to send requested_gas, which would result in an OUT_OF_GAS_ERROR

Therefore, the base cost is itself is 700 gas + access cost(for not touched address will be 2600, Refer this for better understanding) so the ensureNotInVaultContext() wont function properly due to insufficient gas.

The contracts using ensureNotInVaultContext() are BalancerPoolTokenPrice.getWeightedPoolTokenPrice(), BalancerPoolTokenPrice.getStablePoolTokenPrice(), BalancerPoolTokenPrice.getTokenPriceFromWeightedPool(), BalancerPoolTokenPrice.getTokenPriceFromStablePool(), AuraBalancerSupply.getProtocolOwnedLiquidityOhm(), AuraBalancerSupply.getProtocolOwnedLiquidityReserves() and reentrancy check wont function properly in these functions.

Impact

the gas limit is too low for staticcall, the static call will fail, leading to unexpected behavior or errors while executing ensureNotInVaultContext() in above listed functions.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/libraries/Balancer/contracts/VaultReentrancyLib.sol#L77

Tool used

Manual Review

Recommendation

According to the Balancer monorepo here, the staticall must be allocated a 10_000 amount of gas.

Change the reentrancy check to the following.

    function ensureNotInVaultContext(IVault vault) internal view {

         . . . some comments

-        // Reduced to 1k gas to avoid large gas cost predictions when using this function in a non-view function
+        // We set the gas limit to 10k for the staticcall to avoid wasting gas when it reverts due to storage modification
-      (, bytes memory revertData) = address(vault).staticcall{gas: 1_000}(
+      (, bytes memory revertData) = address(vault).staticcall{gas: 10_000}(
            abi.encodeWithSelector(vault.manageUserBalance.selector, 0)
        );

        _require(revertData.length == 0, Errors.REENTRANCY);
    }

[securitygrid]

Escalate VaultReentrancyLib.sol is out of scope, so this is unvalid.

[sherlock-admin2]

Escalate VaultReentrancyLib.sol is out of scope, so this is unvalid.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[10xhash]

The issue is invalid. The idea behind the VaultReentrancyLib implementation is that the call will fail with data.length == 0 in case of a non-reentrant call and with non-zero data in case of a reentrant call. Even with 1k gas, this is not affected. In case of a reentrant call, the storage slot must be warm (because the attacker has set the reentrant flag). Unlike mentioned in the report, the base cost of .staticcall is 100 and with 1k gas, the condition to check whether reentrancy have occured will execute and revert with non-zero data length making the vaultReentrancyLib identify the reentrancy. In case the call is non-reentrant the revert returns no data.

[nevillehuang]

@0xJem I think @10xhash is correct that this issue is invalid, you may want to run some tests to verify his comments and see if a fix is required.

[0xJem]

@10xhash thanks for the clarification

[jacksanford1]

PR fix: https://github.com/OlympusDAO/bophades/pull/244

[0xMR0]

Hi Everyone,

The 1k gas limit may seems to be reasonable for now but depending on the expected lifetime of the contract may get problematic when Ethereum gas economics change in the future. Its not recommended to keep such low gas limit especially for low level function like staticcall and balancer team purposely kept 10k gas limit to avoid issues. Please refer below reasoning,

1) The ethereum gas economics has been changed with time and its very much possible to change in coming days with new EIPs being proposed which could render the contract system unusable in the future. Keeping gas limit as hardcoded will always be problamatic. After EIP-150, low level functions like call,delegatecall base _cost were increased greatly. Refering this docs

EIP-150 increased the base_cost of the CALL opcode from 40 to 700 gas, but most contracts in use at the time were sending available_gas - 40 with every call. So, when base_cost increased, these contracts were suddenly trying to send more gas than they had left (requested_gas > remaining_gas). To avoid breaking these contracts, if the requested_gas is more than remaining_gas, we send all_but_one_64th of remaining_gas instead of trying to send requested_gas, which would result in an OUT_OF_GAS_ERROR.

As you see, there is great increase in base_cost, from 40 to 700, Therefore, it is very much possible that staticcall base_cost may be increased in future, it may be 1500, 2000 which can not be predicted, even this happens the contracts will break especially the VaultReentrancyLib.ensureNotInVaultContext() as it will consider old base cost which would fail after implementing new EIP like the gas cost got changed after EIP-150.

2) Per Solidity docs,

Due to the fact that the EVM considers a call to a non-existing contract to always succeed, Solidity includes an extra check using the extcodesize opcode when performing external calls. This ensures that the contract that is about to be called either actually exists (it contains code) or an exception is raised.

The low-level calls which operate on addresses rather than contract instances (i.e. .call(), .delegatecall(), .staticcall(), .send() and .transfer()) do not include this check, which makes them cheaper in terms of gas but also less safe.

If the contract existence check is kept bydefault for .staticcall() then the gas cost will vary. This could happen in Ethereum.

I would recommend to keep 10k gas limit as Balancer team did, so all such possible issues can be avoided. The issue should be considered valid as kept before.

Thank you!

[Czar102]

It is an issue that may arise in the future, when the parameters of opcodes arbitrarily change, which is out of scope of the considerations. Planning to accept the escalation and consider this issue low/informational.

[0xMR0]

Per Balancer,

        // This staticcall always reverts, but we need to make sure it doesn't fail due to a re-entrancy attack.
        // 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 . . . . . 

      . . . some comments

        (, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }(

Isn't the current implementation used in contracts different from balancer VaultReentrancyLib.sol library?

Earlier balancer had kept 100k as hardcoded gas limit but later reduced to 10k. This 10k gas limit is kept purposely so that the function should not fail. This can be checked here

This was the balancer reasoning for reducing gas limit and it can be checked here

bugfix was applied to reduce the gas limit in the ensureNotInVaultContext function within the VaultReentrancyLib

This means that, Balancer checked multiple scenarios before deciding the gas limit from 100k to 10k. I believe this is enough to warrant this issue as valid Medium since its completely different from balancer specifications/implementation which it should not while checking re-entrancy.

[Czar102]

To consider this issue a Medium, I'd need to see a scenario where the current gas limit is insufficient. Otherwise, this report is too speculative to be considered a vulnerability.

[0xMR0]

Can @0xJem explain, why the gas limit is reduced to 1k instead of 10k as specified by balancer?

[0xJem]

It was imported from one of the Balancer repos at a commit which had the gas limit set at 1000

[0xMR0]

It was imported from one of the Balancer repos at a commit which had the gas limit set at 1000

@0xJem I could find only this for VaultReentrancyLib.sol where Balancer had changed 100k gas limit to 10k.

OR you could share a reference link here where 1k gas limit has been implemented by Balancer?

What i see, the developer has edited the original VaultReentrancyLib.sol and changed the gas limit to 1k in function but forgot to change the gas limit comments. See below comment from currently implemented VaultReentrancyLib.sol where he forgot to change the comment,

 We set the gas limit to 100k, but the exact number . . . . . . 

consider this correct libarary in contracts.

@Czar102 From the above, It is clear that, the developer has used incorrect balancer VaultReentrancyLib.sol with 1k gas limit even which does not exist when i researched, though sponsor claims to be imported from balancer. This is clearly deviating from the balancer VaultReentrancyLibs.sol implementation for checking the re-entrancy. 10k gas limit has been considered by balancer correctly.

[Czar102]

As I said earlier, it doesn't matter what was Balancer's intention. As long as there will be no demonstration of how the contract may misbehave on current EVM versions, this finding will be considered informational.

[Czar102]

Can the reentrancy slot be cold at the time of the execution? Is this function used in an in-scope contract? @nevillehuang @IAm0x52 @0xJem

[0xJem]

Yes it is used in some of the in-scope contracts:

• BalancerPoolTokenPrice
• AuraBalancerSupply

And with the audit fixes, BLVaultSupply

[Czar102]

@0xMR0 @nevillehuang Can the reentrancy slot be cold at the time of the execution? I want to assume eip-2930 transactions aren't intended to be used and the Balancer contracts shouldn't have to be interacted with for the transaction to succeed. Since, this is crucial for determining the validity.

Please note that (to my knowledge) foundry assumes all slots and addresses are warm.

[0xMR0]

It was imported from one of the Balancer repos at a commit which had the gas limit set at 1000

@0xJem I could find only this for VaultReentrancyLib.sol where Balancer had changed 100k gas limit to 10k.

OR you could share a reference link here where 1k gas limit has been implemented by Balancer?

What i see, the developer has edited the original VaultReentrancyLib.sol and changed the gas limit to 1k in function but forgot to change the gas limit comments. See below comment from currently implemented VaultReentrancyLib.sol where he forgot to change the comment,

 We set the gas limit to 100k, but the exact number . . . . . . 

consider this correct libarary in contracts.

@Czar102 From the above, It is clear that, the developer has used incorrect balancer VaultReentrancyLib.sol with 1k gas limit even which does not exist when i researched, though sponsor claims to be imported from balancer. This is clearly deviating from the balancer VaultReentrancyLibs.sol implementation for checking the re-entrancy. 10k gas limit has been considered by balancer correctly.

@0xJem could you please share the 1k gas limit balancer import link?

[0xMR0]

As I said earlier, it doesn't matter what was Balancer's intention. As long as there will be no demonstration of how the contract may misbehave on current EVM versions, this finding will be considered informational.

Sorry, it is balancers intention or i would say recommendation to fix a issue and the use of VaultReentrancyLib.sol library with specified gas limit i.e 10_000 with staticcall is from balancer standard library and this library is recommended to use in functions that are vulnerable to the read-only reentrancy.

 * 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).

I have already given references for both(first incorrect use of VaultReentrancyLib.sol library with incorrect gas limit for which sponsor has not replied with his used reference link) and second(considering now and future issues).

The issue should be judged based on current balancer implementation and bug fix by balancer resulting in 10k gas limit in VaultReentrancyLib.sol library. Please note, balancer could mention to use any arbitrary gas limit but it didnt as the library is tested by balancer. The opcode explanation above was an addition considering to future possibilities.

I am not sure, how it is being considered as speculative submission where as the above explanation is fact based considering balancer changes, etc.

[securitygrid]

1000 gas is enough.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import "forge-std/Test.sol";
import "./interface.sol";

interface IAsset {
    // solhint-disable-previous-line no-empty-blocks
}
interface IVault {
    enum UserBalanceOpKind {
        DEPOSIT_INTERNAL,
        WITHDRAW_INTERNAL,
        TRANSFER_INTERNAL,
        TRANSFER_EXTERNAL
    }
    struct UserBalanceOp {
        UserBalanceOpKind kind;
        IAsset asset;
        uint256 amount;
        address sender;
        address payable recipient;
    }
    function manageUserBalance(UserBalanceOp[] memory ops) external payable;
}

contract Hello is DSTest {
    IVault constant BAL_VAULT = IVault(0xBA12222222228d8Ba445958a75a0704d566BF2C8);

    CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
    function setUp() public {
        cheats.createSelectFork("https://rpc.ankr.com/eth", 19034894);
    }
    function ensureNotInVaultContext(IVault vault) internal view {
        (, bytes memory revertData) = address(vault).staticcall{gas: 1_000}(
            abi.encodeWithSelector(vault.manageUserBalance.selector, 0)
        );

        require(revertData.length == 0, "ERROR");
    }

    function testWhetherOutOfGas() public {
        ensureNotInVaultContext(BAL_VAULT);
    }
}
/* output
Running 1 test for src/test/balancerV2Test.sol:Hello
[PASS] testWhetherOutOfGas() (gas: 4340)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 491.29ms
*/

So this is not an issue in current EVM.

[0xJem]

@0xJem could you please share the 1k gas limit balancer import link?

I am unable to find it - I didn't import this particular library, so don't have the commit in my browser history.

[nevillehuang]

@0xMR0 @nevillehuang Can the reentrancy slot be cold at the time of the execution? I want to assume eip-2930 transactions aren't intended to be used and the Balancer contracts shouldn't have to be interacted with for the transaction to succeed. Since, this is crucial for determining the validity.

Please note that (to my knowledge) foundry assumes all slots and addresses are warm.

@Czar102 I am unable to verify the given integrations of the functions are out of scope of this contest. But based on @securitygrid test, I think this issue is invalid.

[Czar102]

Hey @securitygrid, thank you for the PoC. How is the reentrancy slot warm in this case? If it was cold, I'm quite sure that the SLOAD opcode would eat 2k+ gas by itself.

Thanks in advance.

[securitygrid]

@Czar102 I escalated this issue due to a library that was out of scope, and later, I realized that this case was valid under the current sherlock rules. But, after seeing some comments, I did not delete this escalation.

Your comment about cold/warm slot read reminded me that indeed, 1000 gas is not possible to check whether the reentrant lock is actually locked. Because in most cases, it is cold slot read. There is only one situation where a warm slot read occurs, and that is when the slot is touched in a tx before interacting with the protocol. Whether this will happen or not, I cannot answer. Sponsors should be able to confirm.

I'd like to share my point: A malicious user initiates a tx including two steps:

1. Interact with BalVault and call back to the malicious contract. This has touched the reentrancy lock.
2. Interact with the olympus protocol. When the flow reaches ensureNotInVaultContext, warm slot read only costs 100 gas. Therefore, 1000 gas can protect such a situation.

[Czar102]

@securitygrid I'm not sure what are you referring to in this attack.

I just realized that 1k gas is actually an extremely great idea – if the contract reentered, the slot is warm, so it is enough to throw an error. Otherwise, the execution reverts without any revert data on an expensive SLOAD, indicating that it wasn't reentrancy that caused the revert – the execution can't reenter if the slot is cold.

The system works!

[securitygrid]

1. Interact with BalVault and call back to the malicious contract. This has touched the reentrancy lock.
2. Interact with the olympus protocol. When the flow reaches ensureNotInVaultContext, warm slot read only costs 100 gas. Therefore, 1000 gas can protect such a situation.

What the above case wants to express is:

if the contract reentered, the slot is warm, so it is enough to throw an error.

Regardless of warm/cold slot, the system works as expected.

[Czar102]

Result: Invalid Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• securitygrid: accepted