search

code-423n4 / 2024-05-predy-validation

0 stars 0 forks source link

Potential Token Loss Due to Rounding Issue in Small Deposits #599

Closed c4-bot-7 closed 4 months ago

c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PredyPool.sol#L222-L228 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/SupplyLogic.sol#L37-L41 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/SupplyLogic.sol#L46-L55 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/ScaledAsset.sol#L37-L45

Vulnerability details

Impact

Small deposits, where _amount * Constants.ONE < tokenState.assetScaler, will not yield any shares, effectively rendering such deposits worthless and leading to potential losses for lenders making small contributions.

The lender transfers their tokens to the contract but receives no shares (mintAmount = 0), resulting in a direct loss of the tokens supplied.

Proof of Concept

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

Code


  if (_isStable) {
            mintAmount = receiveTokenAndMintBond(pair.quotePool, _amount);
        } else {
            mintAmount = receiveTokenAndMintBond(pair.basePool, _amount);
        }

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/SupplyLogic.sol#L37-L41


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);
    }

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/SupplyLogic.sol#L46-L55


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;
    }

POC

As per the sponsors commend

yes, assetScaler is always larger than Constants.ONE.

Calculation of claimAmount:

Token Transfer:

The lender's tokens are transferred to the contract regardless of the claimAmount.


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

Minting Shares:

If claimAmount is 0, mintAmount will also be 0, meaning the lender receives no shares in return for the transferred tokens.


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

Example scenario:

Let's assume

FixedPointMathLib.mulDivDown(_amount, Constants.ONE, tokenState.assetScaler) as per this logic

claimAmount = 1*1e18 /1e20 = 0 Because rounding issues.

When the supplied amount of tokens, scaled by Constants.ONE, is less than tokenState.assetScaler, the calculated claimAmount becomes 0 due to rounding down. This results in the lender's tokens being transferred to the contract without receiving any shares in return, leading to a potential loss of the tokens.

Tools Used

Manual Audit

Recommended Mitigation Steps

Enforce the claimAmount check in addAsset() function. Claim amount should be always greater than 0.


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

        claimAmount = FixedPointMathLib.mulDivDown(_amount, Constants.ONE, tokenState.assetScaler);
+  require(claimAmount > 0, " 0 claim amount");
        tokenState.totalCompoundDeposited += claimAmount;
    }

Assessed type

Other

sathishpic22 commented 4 months ago

Hi @alex-ppg

yes, assetScaler is always larger than Constants.ONE.

According to the sponsor's comment, this issue can result in lenders losing their shares when attempting to deposit small amounts. The problematic code is as follows:

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

In the current implementation, without a check on claimAmount, lenders risk losing funds whenever _amount * Constants.ONE is less than tokenState.assetScaler. This is because:

This means that under certain scenarios, particularly when the pool is performing well or the interest rates are high (resulting in a higher assetScaler), lenders could lose their deposits without receiving any corresponding shares.


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

Thank you

alex-ppg commented 4 months ago

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

I do not foresee any HM-level issue arising from the described behavior, and truncation of minuscule amounts is an expected trait of these systems.