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

Error in maxMint calculation #370

Open
c4-bot-8 opened this issue Apr 22, 2024 · 8 comments
Open

Error in maxMint calculation #370

c4-bot-8 opened this issue Apr 22, 2024 · 8 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-501 grade-b Q-23 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_61_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L446

Vulnerability details

The calculation of the maximum shares received for a deposit in CollateralTracker.sol is incorrect.

In the mint() function, the assets received for a mint are being calculated using the previewMint() function, which has a mismatch with the current maxMint() function in the scenario in which the max possible assets are being deposited for the mint.

Impact

Detailed description of the impact of this finding.

Proof of Concept

Currently the maxMint() function calculates the max shares as follows :

 maxShares = (((type(uint104).max)*totalSupply()/totalAssets())*DECIMALS)/(DECIMALS+COMMISSION_FEE)

and previewMint() calculates the assets received as :

assets = (shares*DECIMALS*totalAssets())/(totalSupply*(DECIMALS-COMISSION_FEE))

In the max mint scenario, let's assume the assets required are maximum, i.e, type(uint104).max :

type(uint104).max = (maxShares*DECIMALS*totalAssets())/(totalSupply*(DECIMALS-COMISSION_FEE))

Simplifying, we get : 

maxShares = (type(uint104).ma0x)*totalSupply*(DECIMALS-COMISSION_FEE)/DECIMALS*totalAssets()

The derived formula does not match the formula used in maxMint() leading to incorrect calculation of shares.

Tools Used

Manual Review

Recommended Mitigation Steps

-            return (convertToShares(type(uint104).max) * DECIMALS) / (DECIMALS + COMMISSION_FEE);
+            return (convertToShares(type(uint104).max) * (DECIMALS - COMMISSION_FEE)) / DECIMALS;


## Assessed type

Error
@c4-bot-8 c4-bot-8 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 Apr 22, 2024
c4-bot-8 added a commit that referenced this issue Apr 22, 2024
@c4-bot-11 c4-bot-11 added the 🤖_61_group AI based duplicate group recommendation label Apr 22, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #553

@c4-judge c4-judge reopened this Apr 25, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

Picodes marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 25, 2024
@dyedm1
Copy link

dyedm1 commented Apr 29, 2024

actually is dup #553 , conf'd there

@dyedm1 dyedm1 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Apr 29, 2024
@c4-judge c4-judge added duplicate-553 and removed primary issue Highest quality submission among a set of duplicates labels Apr 29, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #553

@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards duplicate-501 and removed duplicate-553 labels Apr 29, 2024
@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 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 9, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 9, 2024

Picodes changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Picodes marked the issue as grade-b

@C4-Staff C4-Staff reopened this May 13, 2024
@C4-Staff C4-Staff added the Q-23 label May 13, 2024
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-501 grade-b Q-23 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_61_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

5 participants