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.

Sign up for GitHub

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

Jump to bottom

Collateral token balance of DepositQueue is missing in RestakeManager::calculateTVLs() #379

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

Collateral token balance of DepositQueue is missing in RestakeManager::calculateTVLs() #379

howlbot-integration bot opened this issue May 9, 2024 · 8 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 Q-42 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue 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/main/contracts/RestakeManager.sol#L352
https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L254

Vulnerability details

Impact

RestakeManager::calculateTVLs() does not include the ERC20 tokens in DepositQueue, leading to a wrong TVL value and more ezETH minted / less assets received when withdrawing than supposed.

Proof of Concept

DepositQueue::sweepERC20() deposits token rewards from DepositQueue to EigenLayer. Thus, it is expected to hold ERC20 token balances to sweep later. As such, these tokens should be accounted for in the calculation of the total TVL to keep a consistent ratio. Otherwise users may mint ezETH without considering these tokens, then DepositQueue::sweepERC20() is called and increases TVL by depositing in EigenLayer and users get a sudden boost of their value, dilluting rewards for past ezETH holders.

Tools Used

Vscode

Recommended Mitigation Steps

Include the token balances of DepositQueue in RestakeManager::calculateTVLs(), similar to what is done with the WithdrawalQueue.

Assessed type

Other
@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_114_group AI based duplicate group recommendation bug Something isn't working 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
@howlbot-integration
0x73696d616f issue #379
cae1650
@c4-judge
Copy link
Contributor

alcueca marked the issue as not a duplicate

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

alcueca changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 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 labels May 16, 2024
@jatinj615
Copy link

jatinj615 commented May 20, 2024 •
edited
Loading

As EigenLayer payments are not yet enabled which means DepositQueue is not supposed to hold any collateral Tokens (as rewards from Eigen Layer) only ETH. which is why the DepositQueue balance is not accumulated in TVL.

@jatinj615 jatinj615 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label May 20, 2024
This was referenced May 22, 2024
Open
Open
Open
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label May 23, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as primary issue

@alcueca
Copy link

alcueca commented May 23, 2024

Accepting the issue as QA, as it deals with a likely future.

@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

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

alcueca marked the issue as grade-a

@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-b

@c4-judge c4-judge added grade-b and removed grade-a labels May 24, 2024
@C4-Staff C4-Staff added the Q-42 label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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 Q-42 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jatinj615 @alcueca @CloudEllie @itsmetechjay @c4-judge @C4-Staff