sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

Ironsidesec - Prize claims with hooks will fail on Arbitrum L2 #158

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

Ironsidesec

high

Prize claims with hooks will fail on Arbitrum L2

Summary

The primary scope of this audit is to learn the effect/impact of pooltogether in multiple L2 chains. And there's an impact on arbitrum gas unit consumption. Imapct: DOS to prize claims, high beacuse of huge liquidity and volume on Arbitrum and prize claim is the important tx of the system. Root cause: fixed constant low hook gas units (150k)

Imagine a pooltogether player or integrator using on mainnet and optimism currently, he is implementing a hook to emit event on hook after the prize claim or even swapping the prize token to stable coin/WETH. If he integrates/takes part in Arbitrum, then these hooks will fail and prize claim will also fail. So it's a DOS to the players/integrators.

Vulnerability Detail

The team says 150K will be enough to mint an NFT for both beforeClaimPrize and afterClaimPrize, but in Arbitrum average nft mint costs above 500k of gas. Example tx are in table section. Even a simple ether transfer on arbitrum costs 170k units, check here. So, hook feature on arbitrum will lead to dos because claim prize will fail for the recipient.

Transactions in Arbitrum are 15 - 20x higher than the L1. So the constant gas limit for Hook actions HOOK_GAS = 150_000 when claiming prizes, will revert Out of Gas due to insuffient gas limit in Arbitrum.

so the average NFT mint in mainnet costs 50k - 150k per mint and that is how hook gas is setup. But on arbitrum it cost > 500k - 800k. So any plain action in the hook will fail due to the low 150k gas limit. It doesn't have to be not mint on arbitrum, even a token swap / event emission will fail.

gas comparisons between Mainnet L1 and Arbitrum L2 action Mainnet example Arbitrum example
nft-1 mint 40k bored ape 800k merky onft
nft-2 mint 110k rabby 700k zk-merkly
Uni V3 nft mint 450k etherscan 2400k arbiscan
ETH transfer 20k etherscan 500k arbiscan
Uniswap v3 swap 250k etherscan 2300k arbiscan
USDT approve 50k etherscan 900k arbiscan
USDT transfer 40k etherscan 900k arbiscan

Impact

Prize claim with hooks on arbitrum will fail due to low gas units allowance. So permanent DOS to hooks feature on Arbitrum

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#L87

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L110

Tool used

Manual Review

Recommendation

Change the gas limit from constant to immutable and set them while deploying on each chain. Or to be more careful and flexible, have a setter function with owner control, to change the gas limit whenever needed, ex: chain upgrades.

Duplicate of #81

sherlock-admin2 commented 5 months ago

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

nevillehuang commented 5 months ago

See comments here