supply and withdraw method lacks slippage mechanism

[c4-bot-7]

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