New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The winner can steal claimer fees, and force him to pay for the gas #345
Comments
Claimable::claimPrize has the visibility of onlyClaimer denying the winner's hook reentrancy. The winner can't any prize unless he/she is the permitted claimer. |
raymondfam marked the issue as insufficient quality report |
raymondfam marked the issue as duplicate of #18 |
hansfriese changed the severity to QA (Quality Assurance) |
hansfriese marked the issue as grade-b |
Hi @hansfriese, thanks for judging this contest in that short period. There is a misunderstanding of this issue with its group ( There are three contracts that we will deal with for making this exploit:
The issue was rejected by replying that the In my report, I said that the hook will go to the Claimer contract itself, and call the
I think the conflict occurs as I illustrated in my report that the way the Claimer contract design is not restricted and, anyone can use it to claim fees and send prizes to the winners.
According to @raymondfam comment for rejecting this issue:
Anyone can be a permitted claimer as the function that interacts with the In my report I did not say that the hook will go to fire
So the pass will be the following: We will use (MEV searcher) to represent the one that claims winners' prizes using Claimer contract
PASS:
I highly encourage setting up the PoC and running it, I made a simulation of the normal state, and when the hook is used to steal rewards. By providing All what I said here exists in my report, and I provided a runnable PoC that can be used by the judge to simulate the attack, it can be viewed by expanding the collapsed content from the triangle, and following setting up PoC instructions. One additional thing: this issue is not a duplicate of issue As I illustrated in the title there are two Impacts:
Sorry for That long escalation text, but I wanted to explain things as well as I could to show the issue and its Impact to the Judger. Please, if the Judger could reread the issue writeup carefully, after taking into consideration the points of conflict I showed here. Thanks a lot, @hansfriese as well as @raymondfam for your time. |
hansfriese removed the grade |
This previously downgraded issue has been upgraded by hansfriese |
Nice report! |
hansfriese marked the issue as not a duplicate |
hansfriese marked the issue as satisfactory |
hansfriese marked the issue as primary issue |
hansfriese marked the issue as selected for report |
Although this issue can be mitigated in the By fixing the issue in the prize pool and claimer, we save gas by avoiding the two additional external calls added in the original mitigation.
|
trmid (sponsor) confirmed |
Lines of code
https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/abstract/Claimable.sol#L86-L92
Vulnerability details
Impact
When the winner earns his reward he can either claim it himself, or he can let a claimer contract withdraw it on his behalf, and he will pay part of his reward for that. This is because the user will not pay for the gas fees, instead the claimer contract will pay it instead.
The problem here is that the winner can make the claimer pay for the gas of the transaction, without paying the fees that the claimer contract takes.
Claimer contracts are allowed for anyone to use them, transfer prizes to winners, and claim some fees. where the one who fired the transaction is the one who will pay for the fees, so he deserved that fees.
pt-v5-claimer/Claimer.sol#L120-L150
As in the function, the function takes winners, and he passed the fees recipient and his fees (but it should not exceeds the maxFees which is initialzed in the constructor).
Now We know that anyone can transfer winners' prizes and claim some fees.
Before the prizes are claimed, the winner can initialize a hook before calling the
PoolPrize::claimPrize
, and this is used if the winner wants to initialize another address as the receiver of the reward.The hook parameter is passed by parameters that are used to determine the correct winner (winner address, tier, prizeIndex).
abstract/Claimable.sol#L85-L95
But to prevent
OOG
the gas is limited to150K
.Now What can the user do to make the claimer pay for the transaction, and not pay any fees is:
beforeClaimPrize
hookClaimer::claimPrizes(...params)
but with settings no fees, and only passing his winning prize parameters (we got them from the hook).OOG
(remember we have only 150k).Note: The Claimer claiming function will not revert, as if the prize was already claimed the function will just emit an event and will not revert
pt-v5-claimer/Claimer.sol#L194-L198
The only Check that can prevent this attack is the gas cost of calling
beforeClaimPrize
hook.We will call one function
Claimer::claimPrizes()
by only passing one winner, and without fees. We calculated the gas that can be used by installing protocol contracts (Claimer and PrizePool), then grap a test function that first the function we need, and we got these results:Claimer::claimPrize()
costs5292 gas
if it did not claimed anything.PrizePool::claimePrize()
costs118124 gas
.So the total gas that can be used is$118,124 + 5292 = 123,416$ . which is smaller than
HOOK_GAS
by more than25K
, so the function will not revert because of OOG error, and the reentrancy will occur.Another thing that may lead to mis-understanding is that the Judger may say ok if this happens the function will go to
beforeClaimPrize
hook again leading to infinite loop and the transaction will goOOG
. But making the transactionbeforeClaimPrize
be fired to make a result and when called again do another logic is an easy task that can be made by implementing a counter or something. However, we did not implement this counter in our test. We just wanted to point out how the attack will work in our POC, but in real interactions, there should be some edge cases to take care of and further configurations to take care off.Proof of Concept
We made a simulation of how the function will occur. We found that the testing environment made by the devs is abstracted a little bit compared to the real flow of transactions in the production mainnet, so I made Mock contracts, and simulated the attack with them. Please go for the testing script step by step, and it will work as intended.
test/Claimable.t.sol::L8
Imports and Contracts
test/Claimable.t.sol::L132
Testing Functions
beforeClaimPrize
hook function, and replace it with the following.Output:
In this test, we first made a reward and withdraw it from our Claimer contract normally (no attack happened), and then we made another prize reward but by making the attack when withdrawing it, which can be seen in the Logs.
Tools Used
Manual Review + Foundry
Recommended Mitigation Steps
We can check the prize state before and after the hook and if it changes from unclaimed to claimed, we can revert the transaction.
NOTE: We was writing this issue 30 min before ending of the contest so we did not check for the grammar quality or the words, and the mitigation review may not be the best, or may not work (we did not tested it), Devs should keep this in mind when mitigating this issue.
Assessed type
Reentrancy
The text was updated successfully, but these errors were encountered: