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

getRsETHAmountToMint() will return incorrect amount to mint. #783

Open
c4-submissions opened this issue Nov 15, 2023 · 7 comments
Open

getRsETHAmountToMint() will return incorrect amount to mint. #783

c4-submissions opened this issue Nov 15, 2023 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-479 grade-b Q-13 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109

Vulnerability details

Impact

In LRTDepositPool.sol the function getRsETHAmountToMint() gets the price of an asset from chainlink via the aggregator and uses it to calculate the rsETH amount to mint for a certain amount of LST, the issue is that different chainlink aggregator feeds have different decimals for different tokens whenever it returns the price, the price should be scaled in order to prevent rounding issues whenever it is divided by a value in 1e18, this can be seen when rsETH in the pool is 0, it returns 1 ether which is Wei in 18 decimal places
Consider an amount of LST = 3 and the price from the feed is 1e8 dividing by 1 ether which is 1e18 will lead to rounding error where
3 * 1e8 / 1e18
The value gotten will be rounded down to zero.

  • Incorrect amounts of rsETH to mint will be returned for tokens with a different decimal place value than rsETH:ETH

Tools Used

Manual review

Recommended Mitigation Steps

Consider the tokens decimal whenever getting the rsETH amount to mint for a particular token.

Assessed type

Decimal

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 15, 2023
c4-submissions added a commit that referenced this issue Nov 15, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Nov 16, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #97

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #479

@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 Dec 1, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Dec 1, 2023

fatherGoose1 changed the severity to 2 (Med Risk)

@c4-judge
Copy link
Contributor

c4-judge commented Dec 1, 2023

fatherGoose1 marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 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 Dec 1, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Dec 4, 2023

fatherGoose1 changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Dec 8, 2023

fatherGoose1 marked the issue as grade-b

@C4-Staff C4-Staff reopened this Dec 8, 2023
@C4-Staff C4-Staff added the Q-13 label Dec 8, 2023
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 duplicate-479 grade-b Q-13 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 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

4 participants