Skip to content
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

An attacker could front-run the ERC20Airdrop.claimAndDelegate function, thereby preventing the user from claiming their funds #201

Closed
c4-bot-3 opened this issue Mar 26, 2024 · 10 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_44_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-3
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop.sol#L71

Vulnerability details

The ERC20Airdrop contract provides only the claimAndDelegate function to claim funds from the contract. The issue arises from the fact that an attacker could front-run the user by calling IVotes.delegateBySig function directly with the signature from the user's transaction. In this case, the user's transaction will revert and the user won't be able to claim their funds. The attacker might do this constantly, effectively preventing the user from claiming funds. This issue becomes even more severe because of these facts:

  1. The user's transactions to claim funds are more costly than the attacker's transactions to prevent it since the revert of the user's transactions happens almost at the end of the execution
  2. The token in question has voting functions so the attacker might have an indirect incentive to perform such an attack. In some cases even delaying claiming might be favorable to an attacker.

It's worth mentioning that users can still protect themselves from this attack by utilizing protected RPC but it requires some additional steps and costs more for the user.

Impact

An attacker might front-run a user's transactions, effectively preventing the user from claiming their funds, or at least delaying the process.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider separating claiming and delegating processes by implementing the claim function that doesn't involve the delegating part.

Assessed type

DoS

@c4-bot-3 c4-bot-3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 26, 2024
c4-bot-6 added a commit that referenced this issue Mar 26, 2024
@c4-bot-12 c4-bot-12 added the 🤖_44_group AI based duplicate group recommendation label Mar 27, 2024
@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Mar 29, 2024
@c4-pre-sort
Copy link

minhquanym marked the issue as primary issue

@c4-pre-sort
Copy link

minhquanym marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 29, 2024
@dantaik
Copy link

dantaik commented Apr 3, 2024

I think this a valid report but the severity of this bug is minor. I've created a PR to address this issue: taikoxyz/taiko-mono#16622

@c4-sponsor
Copy link

dantaik (sponsor) confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Apr 3, 2024
@c4-sponsor
Copy link

dantaik marked the issue as disagree with severity

@c4-sponsor c4-sponsor added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Apr 3, 2024
@adaki2004
Copy link

@dantaik I'd agree with the severity. The audited version a person could setup a server and a node, frontrunnig every transaction, theoretically blocking the whole system. (I know it is fixed, but i'd accept medium).

@c4-sponsor
Copy link

adaki2004 marked the issue as agree with severity

@c4-sponsor c4-sponsor removed the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Apr 4, 2024
@0xean
Copy link

0xean commented Apr 9, 2024

downgrading to QA, these hypothetical DOS attacks have no economic incentive to the attacker and I have yet to see proof of them occurring in the wild.

@c4-judge
Copy link
Contributor

c4-judge commented Apr 9, 2024

0xean changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax grade-b and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Apr 9, 2024
@c4-judge
Copy link
Contributor

0xean marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_44_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

9 participants