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

Base Tokens in Pair Contract are Assumed to Have 1e18 decimals #277

Closed
code423n4 opened this issue Dec 19, 2022 · 4 comments
Closed

Base Tokens in Pair Contract are Assumed to Have 1e18 decimals #277

code423n4 opened this issue Dec 19, 2022 · 4 comments
Labels
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 downgraded by judge Judge downgraded the risk level of this issue duplicate-141 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L391
https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L20

Vulnerability details

Description

The price() function in the Pair.sol contract is designed to calculate the current price of one fractional token in base tokens with 18 decimals of precision however, there is a flaw in the price implementation where tokens such as USDT or USDC have not been considered which are of 6 decimal places.

Impact

This was awarded a High in severity because if the base token for a pair is not of 18 decimal places, this may result in an immediate loss of funds or undeserved gains when calculating token prices. Since the price() function assumes that the base token reserves are of 18 decimal places, internal accounting may be deemed incorrect if a token such as USDC (1e6) was to be used as the base reserve token. Therefore, because the price of a base token is being multiplied by 1e18 and divided by the amount of fractional tokens, malicious actors may be able to deposit a minimal amount and withdraw a large amount of tokens which may result in financial loss for users invested in the pair. In addition to this, fractional tokens may appear to be worth more than they actually are.

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L391

Recommended Mitigation Steps

It's recommended that when performing operations involving decimal precision, the base token reserves should be multiplied dynamically by using ERC20(baseToken).decimals() to cater for a wider range of tokens.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 19, 2022
code423n4 added a commit that referenced this issue Dec 19, 2022
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as duplicate of #53

C4-Staff added a commit that referenced this issue Jan 6, 2023
@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 Jan 10, 2023
@c4-judge
Copy link
Contributor

berndartmueller changed the severity to 2 (Med Risk)

@c4-judge
Copy link
Contributor

berndartmueller marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 10, 2023
@C4-Staff
Copy link
Contributor

CloudEllie marked the issue as duplicate of #141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 downgraded by judge Judge downgraded the risk level of this issue duplicate-141 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants