No slippage control for supply and withdraw functions in PredyPool

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

When a user supplies quoteTokens or baseTokens, they get bond tokens in return, representing their share in the pool. The problem is there is no slippage check on the amount of bond tokens that a user wants to receive in return. This could result in the user receiving fewer bond tokens than intended.

Proof of Concept

When a user calls supply function within the PredyPool, it internally calls the
supply function in SupplyLogic, which then calls the receiveTokenAndMintBond function:

    function receiveTokenAndMintBond(Perp.AssetPoolStatus storage _pool, uint256 _amount)
        internal
        returns (uint256 mintAmount)
    {
        mintAmount = _pool.tokenStatus.addAsset(_amount);

        ERC20(_pool.token).safeTransferFrom(msg.sender, address(this), _amount);

        ISupplyToken(_pool.supplyTokenAddress).mint(msg.sender, mintAmount);
    }

The amount to be minted is decided by the addAsset function. addAsset is defined as:

    function addAsset(AssetStatus storage tokenState, uint256 _amount) internal returns (uint256 claimAmount) {
        if (_amount == 0) {
            return 0;
        }

        claimAmount = FixedPointMathLib.mulDivDown(_amount, Constants.ONE, tokenState.assetScaler);

        tokenState.totalCompoundDeposited += claimAmount;
    }

It returns the value claimAmount which is the amount being minted. claimAmount is calculated as FixedPointMathLib.mulDivDown(_amount, Constants.ONE, tokenState.assetScaler);. It is dependent on the value tokenState.assetScaler.

assetScaler gets updated every time updateScaler is called in the following lines:

        uint256 supplyInterestRate = FixedPointMathLib.mulDivDown(
            FixedPointMathLib.mulDivDown(
                _interestRate, getTotalDebtValue(tokenState), getTotalCollateralValue(tokenState)
            ),
            100 - _reserveFactor,
            100
        );

        tokenState.debtGrowth += _interestRate;
        tokenState.assetScaler =
            FixedPointMathLib.mulDivDown(tokenState.assetScaler, Constants.ONE + supplyInterestRate, Constants.ONE);
        tokenState.assetGrowth += supplyInterestRate;

Note 1: assetScaler is dependent on supplyInterestRate which is dependent on
getTotalDebtValue and getTotalCollateralValue.

Note 2: updateScaler is called inside the applyInterestForPoolStatus function. applyInterestForPoolStatus is called within applyInterestForToken. applyInterestForToken is called every time a supply, withdraw or trade transaction is executed. So, every time a supply, withdraw or trade transaction is executed, the assetScaler value is also updated.

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 ⬇️

Had there been no withdrawal or trade transaction before the supply transaction, the user would have got more bond tokens. Hence, the bond tokens intended for the user with the supply transaction will decrease if there was a previous withdraw transaction in the same block. As there is no slippage control, the user cannot control the minimum amount of bond tokens he wants.

Tools Used

Manual review

Recommended Mitigation Steps

Introduce slippage control in supply and withdraw functions in PredyPool

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c