ctf_sec - Decimal conversion accounting issue in MarketCore#_processAllBatchedEpochActions

[sherlock-admin]

ctf_sec

medium

Decimal conversion accounting issue in MarketCore#_processAllBatchedEpochActions

Summary

Decimal conversion accounting has issue in MarketCore#_processAllBatchedEpochActions

Vulnerability Detail

The PoolToken has a decimal 18, but chainlink price feed may have decimal of 8 or 18,

The payment token may have different decimals.

Some tokens have low decimals (e.g. USDC has 6). Even more extreme, some tokens like Gemini USD only have 2 decimals.

USDT has 6 decimals. NEAR token has 24 decimals.

then without properly scaling or converting the scale of the token, the code would fail in

MarketCore.sol#_processAllBatchedEpochActions

    // Only if mints or redeems exist is it necessary to adjust supply and collateral.
    if (batch.paymentToken_deposit > 0 || batch.poolToken_redeem > 0) {
      changeInMarketValue_inPaymentToken = int128(batch.paymentToken_deposit) - int256(uint256(batch.poolToken_redeem).mul(price));

      int256 changeInSupply_poolToken = int256(uint256(batch.paymentToken_deposit).div(price)) - int128(batch.poolToken_redeem);

      pool.batchedAmount[associatedEpochIndex & 1] = BatchedActions(0, 0);

      _handleChangeInPoolTokensTotalSupply(poolToken, changeInSupply_poolToken);
    }

the code can just underflow:

batch.paymentToken_deposit could be 1000 * 10e6 (6 decimal token)

batch.poolToken_redeem = 900 10e18 (18 decimals) 11 * 10e8 (chainlink price has 8 decimals)

clearly, the code below revert in underflow.

      int256 changeInSupply_poolToken = int256(uint256(batch.paymentToken_deposit).div(price)) - int128(batch.poolToken_redeem);

Impact

value depending on the batched deposits and redeems that took place during the epoch will be incorrectly updated.

Code Snippet

https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L563-L586

Tool used

Manual Review

Recommendation

I think the project should scale the token by decimal properly.

For example,

The project can change from

changeInMarketValue_inPaymentToken = int128(batch.paymentToken_deposit) - int256(uint256(batch.poolToken_redeem).mul(price));

to

paymentToken_deposit = batch.paymentToken_deposit / (10 ** PaymentToken_decimals)
poolToken_redeem = batch.poolToken_redeem / (10 ** 18)
changeInMarketValue_inPaymentToken = (paymentToken_deposit - poolToken_redeem).mul(price)

Duplicate of #21

[JasoonS]

Yes - we should've removed USDC from the README (here also: https://github.com/sherlock-audit/2022-11-float-capital-judging/issues/21). - We would need to fix the scaling so numbers are nice for humans to read if we decided to use decimals other than 18. But the maths wouldn't break we'd just need to keep track of the minimums and be strict with them - to maintain integrity even with payment tokens of different decimals. Mabye scaling the payment token to always be base 18 would work - but maybe not worth the complication if we don't intend on using other payment tokens?

Chainlink decimals have no effect on the mechanism - we always calculate the percentage change of the price feed - percentage change isn't effected by number of decimals.

We could add a require(paymentToken.decimals() == 18) on construction or we could scale the numbers so that they are always base 18 possibly.

[JasoonS]

[Evert0x]

Tagging as a duplicate of #21 as the both address the core issue that the protocol is not built for USDC.

[JasoonS]

https://github.com/Float-Capital/monorepo/pull/3812 more strict checks. Relevant comment: https://github.com/sherlock-audit/2022-11-float-capital/pull/10#discussion_r1035993867