carrotsmuggler - OOG / unexpected reverts due to incorrect usage of staticcall.

[sherlock-admin]

carrotsmuggler

medium

OOG / unexpected reverts due to incorrect usage of staticcall.

Summary

OOG / unexpected reverts due to incorrect usage of staticcall.

Vulnerability Detail

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.

The 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.

This can also be shown with a simple POC.

unction testAttack() public {
        mockRootPrice(WSTETH, 1_123_300_000_000_000_000); //wstETH
        mockRootPrice(CBETH, 1_034_300_000_000_000_000); //cbETH

        IBalancerMetaStablePool pool = IBalancerMetaStablePool(WSTETH_CBETH_POOL);

        address[] memory assets = new address[](2);
        assets[0] = WSTETH;
        assets[1] = CBETH;
        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 10_000 ether;
        amounts[1] = 0;

        IBalancerVault.JoinPoolRequest memory joinRequest = IBalancerVault.JoinPoolRequest({
            assets: assets,
            maxAmountsIn: amounts, // maxAmountsIn,
            userData: abi.encode(
                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,
                amounts, //maxAmountsIn,
                0
            ),
            fromInternalBalance: false
        });

        IBalancerVault.SingleSwap memory swapRequest = IBalancerVault.SingleSwap({
            poolId: 0x9c6d47ff73e0f5e51be5fd53236e3f595c5793f200020000000000000000042c,
            kind: IBalancerVault.SwapKind.GIVEN_IN,
            assetIn: WSTETH,
            assetOut: CBETH,
            amount: amounts[0],
            userData: abi.encode(
                IBalancerVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,
                amounts, //maxAmountsIn,
                0
            )
        });

        IBalancerVault.FundManagement memory funds = IBalancerVault.FundManagement({
            sender: address(this),
            fromInternalBalance: false,
            recipient: payable(address(this)),
            toInternalBalance: false
        });

        emit log_named_uint("Gas before price1", gasleft());
        uint256 price1 = oracle.getPriceInEth(WSTETH_CBETH_POOL);
        emit log_named_uint("price1", price1);
        emit log_named_uint("Gas after price1 ", gasleft());
    }

The 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.

The output shows this:

[PASS] testAttack() (gas: 9203730962297323943)
Logs:
Gas before price1: 9223372036854745204
price1: 1006294352158612428
Gas after price1 : 425625349158468958

This shows that 96% of the gas sent is burnt up in the oracle call.

Impact

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.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/libs/BalancerUtilities.sol#L19-L28

Tool used

Foundry

Recommendation

According to the monorepo here, the staticall must be allocated a fixed amount of gas. Change the reentrancy check to the following.

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

This ensures gas isn't burnt up without reason.

[JeffCX]

Escalate

politely dispute that the severity is high because the transaction that meant to check the reentrancy burn too much gas and can revert and can block withdraw of fund or at least constantly burn all amount of gas and make user lose money

in a single transaction, the cost burnt can by minimal, but suppose the user send 10000 transaction, the gas burnt lose add up

[sherlock-admin2]

Escalate

politely dispute that the severity is high because the transaction that meant to check the reentrancy burn too much gas and can revert and can block withdraw of fund or at least constantly burn all amount of gas and make user lose money

in a single transaction, the cost burnt can by minimal, but suppose the user send 10000 transaction, the gas burnt lose add up

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.

[sherlock-admin2]

Escalate

This issue should be a Low.

The reason is that it will only consume all gas if the staticcall reverts, which means a re-entrancy is detected. For normal usage of the application, there will not be any re-entrancy. Thus, the normal users who use the protocol in the intended manner will not be affected by this issue.

It will only consume all the gas of attackers trying to carry out a re-entrancy attack against the protocol. In this case, the attacker will not get the gas refund when the re-entrancy detection reverts.

The loss of gas refunding for the attacker is not a valid issue in Sherlock since we are protecting the users, not the malicious users attempting to perform a re-entrancy attack or someone using the features in an unintended manner that triggers the re-entrancy revert. In addition, the loss of gas refund is not substantial enough to be considered a Medium since the chance of innocent users triggering a re-entrancy is close to zero in real life.

From the perspective of a smart contract, if an innocent external contract accidentally calls the Balancer protocol that passes the control to the contract and then calls Tokemak, which triggers a re-entrancy revert, such a contract is not operating correctly and should be fixed.

You've deleted an escalation for this issue.

[JeffCX]

With full respect to senior watson's comment

The reason is that it will only consume all gas if the staticcall reverts, which means a re-entrancy is detected

I have to dispute the statement above,

Please review the duplicate issue #837 as well

the old balancer reentrancy check version does not cap the staticcall gas limit

but the new version add the 10000 gas cap

https://github.com/balancer/balancer-v2-monorepo/blob/227683919a7031615c0bc7f144666cdf3883d212/pkg/pool-utils/contracts/lib/VaultReentrancyLib.sol#L43-L55

and the balancer team already clearly state that the static call always revert even the reentrancy is not detected

// However, use a static call so that it can be a view function (even though the function is non-view).
// This allows the library to be used more widely, as some functions that need to be protected might be
// view.
//
// 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 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:
//
//    function _enterNonReentrant() private {
//        // If the Vault is actually being reentered, it will revert in the first line, at the `_require` that
//        // checks the reentrancy flag, with "BAL#400" (corresponding to Errors.REENTRANCY) in the revertData.
//        // The full revertData will be: `abi.encodeWithSignature("Error(string)", "BAL#400")`.
//        _require(_status != _ENTERED, Errors.REENTRANCY);
//
//        // If the Vault is not being reentered, the check above will pass: but it will *still* revert,
//        // because the next line attempts to modify storage during a staticcall. However, this type of
//        // failure results in empty revertData.
//        _status = _ENTERED;
//    }

use not capping the gas limit of static call means the user are constantly waste too much gas (let us say for a single call the user waste and lose 0.01 ETH at gas), after 20000 transaction the loss is cumulatively 200 ETH and has no upper limit of loss

because the reason above

the balancer push a PR fix for this issue specifically, can see the PR

balancer/balancer-v2-monorepo#2467

gas is either wasted or transaction revert and block withdraw, which is both really bad for user in long term, so the severity should be high instead of medium

//  If the Vault is not being reentered, the check above will pass: but it will *still* revert,
//  because the next line attempts to modify storage during a staticcall. However, this type of
//  failure results in empty revertData.

[carrotsmuggler]

The comment by the LSW is wrong. The POC clearly shows 90% of gas is consumed even when no re-entrancy is detected, i.e. for normal usage of the protocol.

When there is reentrancy, the entire transaction reverts. If there is no reentrancy, the static call still reverts due to a state change. There is no reentrancy for the situation given in the POC.

The manageUserBalance call always does a state change. When a state change is encountered during a static call, the entire gas is burnt up and the execution reverts. This happens irrespective of reentrancy conditions.

[xiaoming9090]

Thanks @JeffCX and @carrotsmuggler for your explanation. You are correct that the issue will happen irrespective of reentrancy conditions. The manageUserBalance function will trigger the _enterNonReentrant modifier. If there is no re-entrancy, this line of code will result in a state change that consume all the gas pass to it. I have deleted the escalation.

[Trumpero]

Hello JeffCX, I believe that the loss of gas might not qualify as a valid high. According to the guidelines in Sherlock's documentation, the OOG matter will be deemed a medium severity issue. It could be considered high only in cases where it results in a complete blockage of all user funds indefinitely.

[JeffCX]

Sir, I agree the OOG does not block user withdraw forever

but because the static call always revert and waste 63/64 gas when withdraw, the remaining 1 / 64 gas has to be enough to complete the transaction.

this means user are force to overpaying 100x more gas to complete the withdraw from balancer vault

we can make a analogy:

the protocol open a bank, user put 10K fund into the bank, and the user should only pays for 1 USD transaction fee when withdraw the fund,

but the bank said, sorry because of a bug, everyone has to pay 100 USD to withdraw the fund, and this 100x cost applies every user in every withdrawal transaction, then the result is the withdraw is not really usable and cause consistent loss of fund

this 100x gas applies to all user in every withdrawal transaction that use the balancer vault and the loss of fund from gas has no upper bound, so I think a high severity is still justified.

[Trumpero]

Hello, @JeffCX,

The Out Of Gas (OOG) situation renders users unable to call a method of the contracts due to insufficient gas. On the other hand, your issue poses a risk where users:

• Couldn't call a method due to insufficient gas
• Users can pay more fees to trigger the function

From what I observe, the impact of your issue appears to be a subset of the impact caused by an OOG. Therefore, if the OOG is considered medium, your issue should be equal to or less than medium in severity. I would appreciate it if you could share your opinion on this.

[JeffCX]

Yeap, it is a subset of impact caused by OOG,

so Users can pay more fees to trigger the function, but as I already shown, every user needs to pay 100x gas more in every withdrawal transaction for balancer vault, so the lose of fund as gas is cumulatively high :)

[Trumpero]

I believe that in the scenario of an OOG/DOS, it represents the worst-case scenario for your issue.
This is because when an OOG/DOS happens, users will pay a gas fee without any results, resulting in a loss of their gas. Hence, the impact of an OOG can be rephrased as follows: "users pay 100x gas fee but can't use the function". On the other hand, your issue states that "users pay 100x gas fee but sometime it fails". Is that correct?

[JeffCX]

user can pay 100x gas and use the function as long as the remaining 1/64 gas can complete the executions.

in my original report

#837

the impact I summarized is:

the function may waste too much gas and result in an out of gas error and can block function call such as withdraw

emm as for

users pay 100x gas fee but sometime it fails

as long as user pay a lot of gas (which they should not, transaction can be processed), and if they do not pay that amount of gas, transaction fails

and sir, just a friendly reminder

my original escalation is the severity should be high instead of medium based on the impact 👍

[Trumpero]

Certainly, I comprehend that you are aiming to elevate the severity level of this issue. However, my stance remains that this issue should be classified as medium due to the following rationale:

Let's consider a situation where Alice intends to initiate 2 methods. Method A results in a denial-of-service (DOS) due to an out-of-gas (OOG) scenario, while method B aligns with your described issue.

1. Alice expends 1 ETH as a gas fee but is unable to execute method A. Even when she attempts to allocate 10 ETH for the gas fee, she still cannot trigger method A.

2. Simultaneously, Alice expends 1 ETH as a gas fee but encounters an inability to execute method B. However, when she allocates 10 ETH for the gas fee, she successfully triggers method B.

Consequently, we observe that method A costs Alice 11 ETH as a gas fee without any return, whereas method B costs Alice the same 11 ETH, yet she gains the opportunity to execute it. Hence, we can infer that method A is more susceptible than method B.

[JeffCX]

Sir, I don't think the method A and method B example applies in the codebase and in this issue

there is only one method for user to withdraw share from the vault

I can add more detail to explain how this impact withdraw using top-down approach

User can withdraw by calling withdraw in LMPVault.sol and triggers _withdraw

the _withdraw calls the method _calcUserWithdrawSharesToBurn

this calls LMPDebt._calcUserWithdrawSharesToBurn

we need to know the debt value by calling destVault.debtValue

this calls this line of code

this calls the oracle code

uint256 price = _systemRegistry.rootPriceOracle().getPriceInEth(_underlying);

then if the dest vault is the balancer vault, balancer reetrancy check is triggered to waste 63 / 64 waste in oracle code

so there is no function A and function B call

as long as user can withdraw and wants to withdraw share from balancer vault, 100x gas overpayment is required

[JeffCX]

I think we can treat this issue same as "transaction missing slippage protection"

missing slippage protection is consider a high severity finding, but user may not lose million in one single transaction, the loss depends on user's trading amount

the loss amount for individual transaction can be small but there are be a lot of user getting frontrunning and the missing slippage cause consistent leak of value

all the above character applies to this finding as well

can refer back to my first analogy

the protocol open a bank, user put 10K fund into the bank, and the user should only pays for 1 USD transaction fee when withdraw the fund,

but the bank said, sorry because of a bug, everyone has to pay 100 USD to withdraw the fund, and this 100x cost applies every user in every withdrawal transaction, then the result is the withdraw is not really usable and cause consistent loss of fund

this 100x gas applies to all user in every withdrawal transaction that use the balancer vault and the loss of fund from gas has no upper bound, so I think a high severity is still justified.

[Evert0x]

I think we can treat this issue same as "transaction missing slippage protection"

You are referring to the gas usage here? Putting a limit on the gas is not a task for the protocol, this is a task for the wallet someone is using.

As the escalation comment states

in a single transaction, the cost burnt can by minimal

Impact is not significant enough for a high severity.

Current opinion is to reject escalation and keep issue medium severity.

[JeffCX]

Putting a limit on the gas is not a task for the protocol

sir, please read the report again, the flawed logic in the code charge user 100x gas in every transaction in every withdrawal

in a single transaction, the cost burnt can by minimal

the most relevant comments is #822 (comment)

and #822 (comment)

idk 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

this cause loss of fund for every user in every transaction for not only you but every user...

[Evert0x]

@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

How to identify a high issue:
Definite loss of funds without limiting external conditions.

But it's not clear how much this will be assuming current mainnet conditions.

[JeffCX]

I write a simpe POC

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

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

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockERC20 is ERC20 {
    constructor()ERC20("MyToken", "MTK")
    {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }
}

interface ICheckRetrancy {
    function checkRentrancy() external;
}

contract RentrancyCheck {


    uint256 state = 10;

    function checkRentrancy() external {
        address(this).staticcall(abi.encodeWithSignature("hihi()"));
    }

    function hihi() public {
        state = 11;
    }

}

contract Vault {

    address balancerAddr;
    bool checkRentrancy;

    constructor(bool _checkRentrancy, address _balancerAddr) {
        checkRentrancy = _checkRentrancy;
        balancerAddr = _balancerAddr;
    }

    function toggleCheck(bool _state) public {
        checkRentrancy = _state;
    }

    function withdraw(address token, uint256 amount) public {

        if(checkRentrancy) {
            ICheckRetrancy(balancerAddr).checkRentrancy();
        }

        IERC20(token).transfer(msg.sender, amount);
    
    }

}


contract CounterTest is Test {

    using stdStorage for StdStorage;
    StdStorage stdlib;

    MockERC20 token;
    Vault vault;
    RentrancyCheck rentrancyCheck;

    address user = vm.addr(5201314);

    function setUp() public {
    
        token = new MockERC20();
        rentrancyCheck = new RentrancyCheck();
        vault = new Vault(false, address(rentrancyCheck));
        token.mint(address(vault), 100000000 ether);

        vm.deal(user, 100 ether);
    
        // vault.toggleCheck(true);

    }

    function testPOC() public {

        uint256 gas = gasleft();
        uint256 amount = 100 ether;
        vault.withdraw(address(token), amount);
        console.log(gas - gasleft());

    }

}

the call is

if check reentrancy flag is true

user withdraw -> 
check reentrancy staticall revert and consume most of the gas 
-> withdraw completed

or

if check reentrancy flag is false

user withdraw ->
-> withdraw completed

note first we do not check the reentrancy

// vault.toggleCheck(true);

we run

forge test -vvv --match-test "testPOC" --fork-url "https://eth.llamarpc.com" --gas-limit 10000000

the gas cost is 42335

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] testPOC() (gas: 45438)
Logs:
  42335

then we uncomment the vault.toggleCheck(true) and check the reentrancy that revert in staticcall

vault.toggleCheck(true);

we run the same test again, this is the output, as we can see the gas cost surge

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] testPOC() (gas: 9554791)
Logs:
  9551688

then we can use this python scirpt to estimate how much gas is overpaid as lost of fund

regular =  42313

overpaid = 9551666

# gas price: 45 gwei -> 0.000000045

cost = 0.000000045 * (overpaid - regular);

print(cost)

the cost is

0.427920885 ETH

in a single withdraw, assume user lost 0.427 ETH,

if 500 user withdraw 20 times each and the total number of transaction is 10000

the lose on gas is 10000 * 0.427 ETH

[JeffCX]

note that the more gas limit user set, the more fund user lose in gas

but we are interested in what the lowest amount of gas limit user that user can set the pay for withdrawal transaction

I did some fuzzing

that number is 1800000 unit of gas

the command to run the test is

forge test -vvv --match-test "testPOC" --fork-url "https://eth.llamarpc.com" --gas-limit 1800000

setting gas limit lower than 1800000 unit of gas is likely to revert in out of gas

under this setting, the overpaid transaction cost is 1730089

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] testPOC() (gas: 1733192)
Logs:
  1730089

in other words,

in each withdrawal for every user, user can lose 0.073 ETH, (1730089 uint of gas * 45 gwei -> 0.000000045 ETH)

assume there are 1000 user, each withdraw 10 times, they make 1000 * 10 = 100_00 transaction

so the total lost is 100_00 * 0.07 = 700 ETH

in reality the gas is more than that because user may use more than 1800000 unit of gas to finalize the withdrawal transaction

[Evert0x]

@JeffCX thanks for putting in the effort to make this estimation.

But 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.

This seems like the key sentence

in each withdrawal for every user, user can lose 0.073 ETH,

This is an extra $100-$150 dollars per withdrawal action.

This 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.

[JeffCX]

Sir, I don't think the method A and method B example applies in the codebase and in this issue

there is only one method for user to withdraw share from the vault

I can add more detail to explain how this impact withdraw using top-down approach

User can withdraw by calling withdraw in LMPVault.sol and triggers _withdraw

the _withdraw calls the method _calcUserWithdrawSharesToBurn

this calls LMPDebt._calcUserWithdrawSharesToBurn

we need to know the debt value by calling destVault.debtValue

this calls this line of code

this calls the oracle code

uint256 price = _systemRegistry.rootPriceOracle().getPriceInEth(_underlying);

then if the dest vault is the balancer vault, balancer reetrancy check is triggered to waste 63 / 64 waste in oracle code

so there is no function A and function B call

as long as user can withdraw and wants to withdraw share from balancer vault, 100x gas overpayment is required

the POC is a simplified flow of this

it is ok to disagree sir:)

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• JEFFCX: rejected