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

DepositQueue#sweepERC20 can be sandwiched #386

Open
howlbot-integration bot opened this issue May 9, 2024 · 10 comments
Open

DepositQueue#sweepERC20 can be sandwiched #386

howlbot-integration bot opened this issue May 9, 2024 · 10 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-b Q-39 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_114_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L274

Vulnerability details

Impact

DepositQueue is expected to receive rewards in any of the collateral tokens. They are expected to be forwarded to OperatorDelegators via sweepERC20.

As the collateral balances of DepositQueue are not included in RestakeManager#calculateTVLs, one can monitor DepositQueue#sweepERC20 transactions, deposit before them, initiate the withdrawal right after (at a higher ezETH exchange rate), and claim it after coolDownPeriod, stealing rewards from honest depositors who have been holding ezETH for a significantly longer duration.

Proof of Concept

  1. Alice sees a sweepERC20 transaction in the mempool;
  2. Alice deposits ETH and mints ezETH;
  3. sweepERC20 transaction is mined;
  4. Alice starts a withdrawal of her ezETH at a higher price, and claims it after coolDownPeriod.

Recommended Mitigation Steps

Include DepositQueue's balance in calculateTVL's, minus the fee that would be deduced during sweepERC20.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_114_group AI based duplicate group recommendation bug Something isn't working edited-by-warden sufficient quality report This report is of sufficient quality labels May 9, 2024
howlbot-integration bot added a commit that referenced this issue May 9, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as not a duplicate

@c4-judge c4-judge reopened this May 16, 2024
@c4-judge c4-judge removed duplicate-381 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 16, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels May 16, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as duplicate of #326

@c4-judge c4-judge added duplicate-326 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels May 16, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 17, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as satisfactory

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue labels May 24, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to 3 (High Risk)

@aslanbekaibimov
Copy link

@alcueca

I believe this should be a duplicate of #383 as long as #383 is a separate issue from #326.

The root cause is "DepositQueue ERC20 balances are not accounted in TVL", and the attack path is "sandwiching sweepERC20".

@c4-judge c4-judge reopened this May 27, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

alcueca 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 and removed 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels May 27, 2024
@c4-judge
Copy link
Contributor

alcueca 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 edited-by-warden grade-b Q-39 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_114_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants