Lenders are unable to withdraw complete collateral

[c4-bot-9]

Lines of code

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

Vulnerability details

Summary

The PredyPool:withdraw() function is used to withdraw either the quoteToken or the baseToken from the lending pool. However, due to a rounding issue, lenders are unable to completely exit the pool

Impact

Lenders are unable to exit completely from the PredyPool

Proof of Concept

https://github.com/code-423n4/2024-05-predy/blob/main/src/PredyPool.sol#L222 Lender supply the tokens via supply method and get equivalent bond shares which represent their actual balance in the predypool.

1. Alice is a lender and supplies amount tokens to the PredyPool

2. Alice will receive (amount * 1e18) / (tokenState.assetScaler) bond tokens in return, referred to as aliceTotalBondShares

3. The tokenState.assetScaler increases after every deposit, withdrawal, and trade. Hence, at any time, the maximum amount of assets a lender can withdraw can be viewed by getAvailableCollateralValue()

    function getAvailableCollateralValue(AssetStatus memory tokenState) internal pure returns (uint256) {
        return getTotalCollateralValue(tokenState) - getTotalDebtValue(tokenState);
    }
    function getTotalCollateralValue(AssetStatus memory tokenState) internal pure returns (uint256) {
        return FixedPointMathLib.mulDivDown(tokenState.totalCompoundDeposited, tokenState.assetScaler, Constants.ONE)
            + tokenState.totalNormalDeposited;
    }

    function getTotalDebtValue(AssetStatus memory tokenState) internal pure returns (uint256) {
        return tokenState.totalNormalBorrowed;
    }

4. At any time, Alice should be able to withdraw (total Alice's bond tokens * tokenState.assetScaler) / 1e18. So, Alice calls the withdraw function to withdraw the above amount.

5. predypool.withdraw() calls removeAsset to compute balances and Alice's total balance.

    function removeAsset(AssetStatus storage tokenState, uint256 _supplyTokenAmount, uint256 _amount)
        internal
        returns (uint256 finalBurnAmount, uint256 finalWithdrawAmount)
    {
        if (_amount == 0) {
            return (0, 0);
        }

        require(_supplyTokenAmount > 0, "S3");

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

        if (_supplyTokenAmount < burnAmount) {
            finalBurnAmount = _supplyTokenAmount;
        } else {
            finalBurnAmount = burnAmount;
        }

        finalWithdrawAmount = FixedPointMathLib.mulDivDown(finalBurnAmount, tokenState.assetScaler, Constants.ONE);

        require(getAvailableCollateralValue(tokenState) >= finalWithdrawAmount, "S0");

        tokenState.totalCompoundDeposited -= finalBurnAmount;
    }

As we can see, it calculates burnAmount from (amount * 1e18) / assetScaler, which will always be less than aliceTotalBondShares (from point 2). So Alice won't be able to withdraw her complete amount ever.

Hence, the redeemed bond tokens will always be less than the maximum amount, leaving a small amount of tokens in the PredyPool for every lender

Tools Used

Manual

Recommended Mitigation Steps

Lenders should be able to exit completely i.e make sure the lender can fully redeem their bond tokens and exit the pool.

Assessed type

Math

[Tri-pathi]

submission explains about the issue due to which user is unable to withdraw complete balance. All steps are pretty well explained.

Please have a second look. Thanks

[alex-ppg]

Hey @Tri-pathi, thanks for contributing to the PJQA process! This represents a validation repository finding and as such was not evaluated by me directly.

The _amount argument of the removeAsset function is in the collateral denomination. As such, the burn amount will be less than the original balance the user acquired when depositing, while the withdrawal will be the full amount.

[Tri-pathi]

@alex-ppg

As such, the burn amount will be less than the original balance the user acquired when depositing, while the withdrawal will be the full amount.

Yes, it will withdraw the amount completely, but the user will be unable to exit completely from Predy. A complete exit means the total bond tokens of the user should be zero in the protocol, but with the current implementation, it is not possible. Every user will have some dust bond tokens remaining.

[alex-ppg]

Hey @Tri-pathi, thank you for your feedback. I do not foresee any high-risk or medium-risk impact arising from having dust of the Predy tokens in one's account. As such, this cannot constitute an HM vulnerability and would be at-best a QA issue. Based on this fact, the submission will remain in the validation repository and no further feedback is expected.