supply and withdraw method lacks slippage mechanism

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The lack of a slippage mechanism in supply and withdraw functions could result in bad trade

Proof of Concept

In the PredyPool contract, both supply and withdraw methods lack a slippage mechanism, which can lead to unfavorable trades for users.

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

    function withdraw(uint256 pairId, bool isQuoteAsset, uint256 withdrawAmount)
        external
        nonReentrant
        returns (uint256 finalBurnAmount, uint256 finalWithdrawAmount)
    {
        return SupplyLogic.withdraw(globalData, pairId, withdrawAmount, isQuoteAsset);
    }

Here’s how the lack of slippage can be exploited:

1. User A wants to supply 100,000,000 USDC and expects a corresponding number of bond shares. However, User's B supply tx executed before User A's supply transaction and deposit a large number of assets, which will significantly increase tokenState.assetScaler and decrease the number of bond shares for User A. As tokenState.assetScale increase based on the utilization i.e deposit , withdrawal and trade and inversely proportional to bond shares wrt supplied asset

2. A similar scenario can occur during withdrawals.

Slippage is a crucial parameter when adding or withdrawing assets from such vaults/pools to protect users from these kinds of attacks/losses

Tools Used

Manual

Recommended Mitigation Steps

Add a mechanism to specify the expected number of bond shares a lender expects from supplying assets, and similarly, the expected amount of assets from a withdrawal. This will protect users from significant changes in the tokenState.assetScaler due to front-running or other manipulations.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The Warden has demonstrated how the Predy Pool does not impose any user-supplied slippage checks on its supply and withdrawal functions.

Slippage checks in the supply function are meaningless (as proven by protocols such as Uniswap V3 not permitting slippage checks on the minted LP position amount) given that the evaluation of a share relies on on-chain conditions that may have changed between a transaction's submission and a transaction's execution in the network. As such, submissions #269 and #223 which solely detail slippage checks in this function have been assigned no reward.

Slippage checks in the withdrawal function, however, are crucial given that a user wishes to make a withdrawal based on an evaluation of the collateral-per-share that may no longer hold true when the withdrawal is performed, similar to how Uniswap V3 permits slippage checks on the amounts withdrawn from an LP position. As such, this aspect of each submission in this duplicate set is correct and a severity rating of medium-risk is appropriate.

[Saptarshi1010]

@alex-ppg , since the protocol said "Front-running through insufficient slippage is OOS" shouldn't this be OOS like the other one's

[pkqs90]

Hey @alex-ppg, maybe I'm missing something here, but I don't think the slippage in supply/withdraw is required in Predy.

Let's first see the use case described in this issue.

User A wants to supply 100,000,000 USDC and expects a corresponding number of bond shares. However, User's B supply tx executed before User A's supply transaction and deposit a large number of assets, which will significantly increase tokenState.assetScaler and decrease the number of bond shares for User A. As tokenState.assetScale increase based on the utilization i.e deposit , withdrawal and trade and inversely proportional to bond shares wrt supplied asset

This is simply incorrect, because user B depositing a large number of assets will NOT increase tokenState.assetScaler at all. tokenState.assetScaler is only increased if fees are collected in Predy. https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/ScaledAsset.sol#L210-L211. This is similar to a ERC4626, where the ratio between asset/shares begins at 1 and increases when more fees are collected.

Since shares * tokenState.assetScaler = assets, even if a user's supply/withdraw is frontrun by any other supply/withdraw transaction, they will not lose any funds, because tokenState.assetScaler is monotonically increasing。

This is different than uniswap LP mint/burn because the amount of LP/tokens that a user can get is related to the slot0 price of uniswap, which is manipulatable, and user may lose funds because of it. However, in this case, tokenState.assetScaler is only increasing, and user can't possibly lose funds. Thus I believe the slippage parameter is not required.

[NishithPat]

The assetScaler is updated every time supply or withdraw is called. The flow is supply -> applyInterestForToken -> applyInterestForPoolStatus -> updateScaler.

This issue explains it better imo - #113

Also, the sponsor agrees with this issue as they acknowledged it - #25

[0xklapouchy]

Hi @alex-ppg,

From my perspective this issue is invalid.

The assetScaler in the context of supply() and withdraw() is only used to determine how much I earned from fees. Additionally, supply() and withdraw() deposit only a single token, not a combination of tokens in any ratio.

It functions similarly to the rewardPerToken in Synthetix staking contracts. If a withdrawal is front-run, it will only recalculate (apply rewards) for the user who is withdrawing, or it might not recalculate at all if the action is done within the same block.

It does not affect the amount a user can supply or withdraw.

So at the end what user can withdraw is always equal the amount he supply + extra from collected fees, and there is no option to manipulate his earned fees per tokens supplied.

[pkqs90]

The assetScaler is updated every time supply or withdraw is called. The flow is supply -> applyInterestForToken -> applyInterestForPoolStatus -> updateScaler.

This issue explains it better imo - #113

Also, the sponsor agrees with this issue as they acknowledged it - #25

Calling applyInterestForToken updates tokenState.assetScaler to include up-to-current fees.

1. During supply, applyInterestForToken is called before minting shares, to make sure new deposited tokens would not benefit from up-to-current fees.
2. During withdraw, applyInterestForToken is called before burning shares, to make sure withdrawn tokens benefit from all up-to-current fees.

Let me quote your issue here #113.

Let's say a user calls the supply function, and a big withdraw or a trade transaction is also included in the same block. These withdraw or trade transactions are executed in a way such that the tokenState.totalCompoundDeposited or tokenStatus.totalNormalDeposited is decreased significantly (whenever withdraw is called, tokenState.totalCompoundDeposited is decreased and tokenStatus.totalNormalDeposited can similarly be decreased when trade is called). This decreases the getTotalCollateralValue. This increases the supplyInterestRate (check the above code block). assetScaler is directly proportional to supplyInterestRate, so assetScaler increases. This causes the claimAmount to decrease as it is inversely proportional to assetScaler(check the second code block). Therefore, the amount of bond tokens to be minted decreases.

So, tokenState.totalCompoundDeposited or tokenStatus.totalNormalDeposited ⬇️ -> getTotalCollateralValue ⬇️ -> supplyInterestRate ⬆️ -> assetScaler ⬆️ -> claimAmount ⬇️ -> bond tokens ⬇️

All of your analysis is correct, bond tokens (amount of minted shares) decrease. However, since assetScaler increases, the total asset that the shares corresponds to does not change, since shareAmount * assetScaler = assetAmount. There is no lost of funds here.

[0xAbhay]

Hi @alex-ppg,

From my perspective this issue is invalid.

The assetScaler in the context of supply() and withdraw() is only used to determine how much I earned from fees. Additionally, supply() and withdraw() deposit only a single token, not a combination of tokens in any ratio.

It functions similarly to the rewardPerToken in Synthetix staking contracts. If a withdrawal is front-run, it will only recalculate (apply rewards) for the user who is withdrawing, or it might not recalculate at all if the action is done within the same block.

It does not affect the amount a user can supply or withdraw.

So at the end what user can withdraw is always equal the amount he supply + extra from collected fees, and there is no option to manipulate his earned fees per tokens supplied.

When supplying and withdrawing tokens, slippage is not necessary because the user flow is simple: if you supply 10 tokenA, you will receive 10 shares of tokenA. When withdrawing, the user simply provides shares of the token and receives fees based on the amount of shares they hold of tokenA. There is no need for slippage, and also an oracle is not required to calculate the tokens.

This issue seems invalid from my perspective.

Thanks @alex-ppg for judging I appreciate your work.

[alex-ppg]

Hey @Saptarshi1010, @pkqs90, @NishithPat, @0xklapouchy, and @0xAbhay; I appreciate everyone's diligent contributions to the PJQA stage!

@Saptarshi1010

The issue details how slippage can not even be defined, so it is still in scope.

@pkqs90

Thanks for your feedback! It is possible to receive less funds than expected from the withdrawal function even if the assetScaler is monotonically increasing due to truncation given that the withdrawal function will perform a multiplication and division of the to-be-withdrawn amount to assess the shares due, and will then convert the shares back to the underlying asset amount.

This conversion can directly result in loss of funds albeit of a lower degree than expected. When supplying tokens, slippage is never required as I detailed in my response, and is generally accepted. When withdrawing, a maximum loss of ONE - assetScaler might be incurred and this loss is "impermanent" in the sense that the underlying shares will not be consumed.

In light of this new analysis, a QA risk level is better suited that aligns with the Sponsor's confirmation (i.e. it is a valid issue) but would render it ineligible for an HM reward.

@NishithPat

The value of bond tokens is nondeterminate and dynamic. As long as a withdrawal would result in the same amount of underlying tokens if performed after the supply operation, slippage is redundant in such a case.

@0xklapouchy

As mentioned above to @pkqs90, the withdrawal might result in fewer funds than expected which differs from other DeFi protocols.

@0xAbhay

See above.

Conclusion

The issue will be downgraded to a QA-level risk as I do not envision an HM-level risk to arise from the lack of slippage. The value extracted cannot be manipulated in a negative direction, and the maximum truncation impact is recoverable (i.e. the underlying bond tokens will not be lost).

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c

[Tri-pathi]

@alex-ppg @pkqs90
I don't understand why supply doesn't need slippage protection.
assetScaler increases based on utilization. Let's consider the following scenario:

• User A wants to supply assets with the current assetScaler1, expecting a certain number of bond tokens: (assetsA * 1e18) / assetScaler1.
• Before A's transaction executes, user B executes a supply transaction and receives bond tokens: (assetsB * 1e18) / assetScaler1
• There can be other trades that increase the assetScaler.
• By the time A's supply transaction is executed, the assetScaler has increased to assetScaler2 (where assetScaler2 > assetScaler1)
• Instead of receiving the expected bond tokens (assetsA * 1e18) / assetScaler1, A will receive fewer bond tokens due to the increased assetScaler.

Conclusion

• A was expecting to benefit from the utilization but ends up receiving the same value and deposiing assets in predy
• B got bond tokens at (assetsB * 1e18) / assetScaler1 and can now withdraw (B's bond tokens * assetScaler2) / 1e18, which will be more than assetsB

This is what slippage protection tries to prevent. Due to A's supply transaction, another user, B, made a profit, and the initial user A will pay gas fees and have a suboptimal total portfolio.

To prevent this, we need slippage protection.

PS: Please correct me if I'm wrong

[alex-ppg]

Hey @Tri-pathi, thank you for your contribution. I strongly advise you to adhere to PJQA guidelines so as to avoid risking your SR role.

The scenario outlined is incorrect because it assumes a fixed price per unit of the bond token. The tokens User A expected have been reduced, however, their underlying value has remained the same meaning that no negative impact has affected the user. The tokens User B will receive when withdrawing will be the same as they deposited as the asset scaler is proportionately increased to ensure the same bond-to-token ratio is maintained.

Similarly, LP positions on Uniswap do not impose any slippage on the LP units minted because their value fluctuates and they will never be less than what the user deposited. Whether a user receives 10 LP units of the USDC/WETH pair or 5 LP units is irrelevant as they can issue a withdrawal and acquire the same tokens they deposited back.

The revised ruling will remain in effect and no further feedback is expected for this submission.