elhaj - Hardcoded Gas for Hooks May Prevent Winners from Receiving Prizes Due to Insufficiency on Different Chains

[sherlock-admin3]

elhaj

medium

Hardcoded Gas for Hooks May Prevent Winners from Receiving Prizes Due to Insufficiency on Different Chains

Summary

• The hardcoded gas limit of 150,000 for winner hooks may be insufficient on certain chains like Arbitrum and zksync , causing prize claim failures. This issue arises due to varying gas consumption and fee models across different networks.

  Vulnerability Detail

  The Claimablecontract has a hardcoded gas limit of HOOK_GAS = 150,000 for executing beforeClaimPrize and afterClaimPrize hooks:

  /// @notice The gas to give to each of the before and after prize claim hooks.
  /// @dev This should be enough gas to mint an NFT if needed.
  uint24 public constant HOOK_GAS = 150_000;

• As you see, the comment assumes that this should be enough gas to mint an NFT
• However, different blockchain networks have varying gas consumption and fee models. For instance, while 150,000 gas might be sufficient on Ethereum, it may not be enough on other chains like Arbitrum or zkSync due to their gas requirements and evolving fee modules.which is mentioned in their docs here
• This discrepancy can cause the hooks to fail on certain chains where the protocol will be deployed, leading to transaction reverts when calling the winner hook when the gas required for these hooks exceeds 150,000 on certain chains for the same action (Minting an Nft), preventing the winner from recieving their prize.

  POC :

• add a mainnet url to fork arbitrum and see how the tx revert due outOfGas in this test :

  function testClaimPrize_bothHooks() public {
  ++      vm.createSelectFork(arb_url);
          PrizeHooks memory bothHooks = PrizeHooks(true, true, hooks);
  
          vm.startPrank(alice);
          claimable.setHooks(bothHooks);
          vm.stopPrank();
  
          mockClaimPrize(alice, 1, 2, prizeRedirectionAddress, 1e17, bob);
  
          vm.expectEmit();
          emit BeforeClaimPrizeCalled(alice, 1, 2, 1e17, bob);
          vm.expectEmit();
          emit AfterClaimPrizeCalled(alice, 1, 2, 9e17, prizeRedirectionAddress, "hook data");
  
          vm.recordLogs();
          claimable.claimPrize(alice, 1, 2, 1e17, bob);
          Vm.Log[] memory logs = vm.getRecordedLogs();
  
          assertEq(logs.length, 2);
      }

• console :

  ├─ [150000] ClaimableTest::afterClaimPrize(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 1, 2, 900000000000000000 [9e17], prizeRedirectionAddress: [0x1dD8E5dC40Be9fB68833C78dd78D3F71EA16f762], 0x686f6f6b2064617461)
  >>  │   │   └─ ← [OutOfGas] EvmError: OutOfGas
      │   └─ ← [Revert] EvmError: Revert

  Impact

• Hooks will fail on certain chains, preventing winners from receiving their prizes.

  Code Snippet

• https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L21
• https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L86-L93
• https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L109-L117

  Tool used

  Manual Review , Foundry Testing

  Recommendation

  • Provide chain-specific configurations to adjust the gas limit for winner hooks appropriately.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

on L2 gas is made of L1 gas and L2 gas__ internal calls only consume the L1 gas i.e opcodes cost

[trmid]

The zkSync doc link provided in the issue appears to be outdated or broken and does not have any info on the topic. Is there some updated information on this?

[nevillehuang]

Request poc

[sherlock-admin4]

PoC requested from @elhajin

Requests remaining: 2

[elhajin]

• The POC provided in the report shows how the claim fails on Arbitrum due to OutOfGas with 150k gas when we run it on an Arbitrum fork.(please note that this depends on different factors and in some cases it may not fail)

• yea the link is broken. I think the docs get updated here the correct link.

• Note that the assumption in this comment :point_down: isn't correct:

on L2 gas is made of L1 gas and L2 gas__ internal calls only consume the L1 gas i.e opcodes cost

Here's a quote from zkSync docs about that:

Do not rely on EVM gas logic ZKsync VM has a different set of computational trade-offs compared to the standard computational model. In practice, this means that the price for opcodes is different to Ethereum. Also, zkEVM contains a different set of opcodes under the hood, and so the “gas” metric of the same set of operations may be different on ZKsync Era and on Ethereum.

For Arbitrum, it's a bit complicated. Gas is calculated as a function of the actual gas consumed on L2 and the L1 calldata cost, which is effectively an L2 view of the L1 gas price. Please refer to the Arbitrum docs for more information on how gas is estimated.

[trmid]

In the Arbitrum doc provided, it is stated that:

It works the same as Ethereum gas, in the sense that every EVM instruction costs the same amount of gas that it would on Ethereum.

The gas price is changed dynamically to reflect the L1 and L2 fees, but the gas used remains EVM equivalent.

The PoC listed in the issue does not fail as described. Adding the fork in the test shown reverts for unrelated reasons, and forking the Arbitrum One network in the setup function in Claimable.t.sol results in no errors whatsoever for the testClaimPrize_bothHooks test. This aligns with live results we see on the current Arbitrum One deployment, with hooks running with similar gas used as on Optimism and Base.

As for ZKSync, the main difference from Ethereum gas computation appears to be the gasPerPubdata field which is set by the caller when sending a tx. This is then added to the gas costs based on the resulting state changes and logs from the tx execution. The gas used by opcodes appears to be very similar with some minor changes. There seems to be no indication that 150k gas would be insufficient for the zkSync execution environment.

[nevillehuang]

@elhajin @trmid Seems like per pooltogether's minting logic, the gas spent will not exceed 150_000 gas on arbitrum But this is unconfirmed based on zksync's computation.

[nevillehuang]

@elhajin Is there any additional information you can provide to prove gas computations can exceed 150_000 on zksync?

[elhajin]

My argument is based on the docs that I quote and for me it's pretty clear that a hardcoded 150k gas can be not enough.and it's explicitly mentioned to not hardcode any gas because zksync have an evolving fee model

[nevillehuang]

@elhajin If there is no conclusive evidence that gas will exceed 150_000 on zksync if contracts are deployed as is, I would have to note this as a future integration issue that is OOS:

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.