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

Missing Slippage Protection in the Supply Function #223

Closed
howlbot-integration bot opened this issue Jun 17, 2024 · 4 comments
Closed

Missing Slippage Protection in the Supply Function #223

howlbot-integration bot opened this issue Jun 17, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-95 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_135_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/PredyPool.sol#L222

Vulnerability details

Impact

The current supply function is susceptible to slippage issues. Slippage occurs when there is a difference between the expected price of a trade and the actual price at which the trade is executed. In the context of the supply function, slippage can lead to users receiving fewer bond tokens than anticipated when supplying either the quote or base tokens. This discrepancy can arise due to fluctuations in the market at the time of the transaction.

function supply(uint256 pairId, bool isQuoteAsset, uint256 supplyAmount)
        external
        nonReentrant
        returns (uint256 finalSuppliedAmount)
    {
        return SupplyLogic.supply(globalData, pairId, supplyAmount, isQuoteAsset);
    }

Proof of Concept

Consider a scenario where a user intends to supply a large amount of the quote token to the pool. Due to the size of the transaction, the supply might affect the pool's balance and, consequently, the interest rates and the bond token minting rate. If the transaction executes without slippage protection, the user might receive fewer bond tokens than expected.

For instance:

Alice supplies 1000 quote tokens.
Expected bond tokens: 950.
Actual Transaction without Slippage Protection:

Market conditions or pool liquidity changes during the transaction.
Alice receives only 900 bond tokens instead of 950.
The user faces an unexpected loss of 50 bond tokens due to slippage.

Tools Used

Manual Review

Recommended Mitigation Steps

Incorporate a mechanism to handle slippage tolerance. This can be achieved by allowing users to specify a minimum acceptable amount of bond tokens they are willing to receive from the transaction, and a deadline.

 /
 * @param minMintAmount The minimum acceptable amount of bond tokens to receive
 * @param deadline The latest time the transaction is valid
 */
function supply(uint256 pairId, bool isQuoteAsset, uint256 supplyAmount, uint256 minMintAmount, uint256 deadline) external nonReentrant returns(uint256 finalSuppliedAmount)
    

Assessed type

Context

@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 🤖_135_group AI based duplicate group recommendation bug Something isn't working duplicate-25 sufficient quality report This report is of sufficient quality labels Jun 17, 2024
howlbot-integration bot added a commit that referenced this issue Jun 17, 2024
@alex-ppg
Copy link

The supply function does not need slippage checks per the primary submission's discussion.

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Jun 28, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as unsatisfactory:
Insufficient proof

@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 Jul 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg marked the issue as grade-c

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-95 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_135_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants