Inconsistent downcasting to uint24 result in loss of precision when adding premium

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L525

Vulnerability details

Impact

Inconsistent downcasting to uint24 result in loss of precision when adding premium

Proof of Concept

when the position is created, the excessive swap amount is added as premium

the code make sure the premium amount is safely downcasted from uint256 to uin24

cache.token0PremiumPortion = Base.uint256ToUint24(
    ((params.marginFrom + cache.amountFromBorrowed - cache.feeAmount - cache.amountSpent) *
        Base.BASIS_POINT) / cache.collateralFrom
);
cache.token1PremiumPortion = Base.uint256ToUint24(
    ((cache.amountReceived + cache.amountToBorrowed + params.marginTo - collateralTo) * Base.BASIS_POINT) /
        collateralTo
);

Here, the function uint256ToUint24 is used

     * @notice Helper function to fit a non-overflow uint256 value to uint24
     * @param value the uint256 value to fit
     * @return result uint24 value that fits
     */
    function uint256ToUint24(uint256 value) internal pure returns (uint24 result) {
        if (value > type(uint24).max) revert Errors.Overflow();
        result = uint24(value);
    }

but when user add premium,

the premium amonut is downcasted to uint24 directly, while the transfered premium amount is in the scale of uint256

(uint256 collateral0, uint256 collateral1) = Base.getRequiredCollateral(lien.liquidity, tickLower, tickUpper);

(uint128 token0Premium, uint128 token1Premium) = Base.getPremium(
    collateral0,
    collateral1,
    lien.token0PremiumPortion,
    lien.token1PremiumPortion
);

liens.updatePremium(
    lienKey,
    uint24(((token0Premium + premium0) * Base.BASIS_POINT) / collateral0),
    uint24(((token1Premium + premium1) * Base.BASIS_POINT) / collateral1)
);

// transfer in added premium
if (premium0 > 0) {
    TransferHelper.safeTransferFrom(token0, msg.sender, address(this), premium0);
}
if (premium1 > 0) {
    TransferHelper.safeTransferFrom(token1, msg.sender, address(this), premium1);
}

uint24 is a relative small number,

the max uint24 is 16777215

this is a very small number

1 ERC20 token with 18 decimals is 1,000,000,000,000,000,000

whether the unsafe downcasting (whether the premium exceed uint24) also depends on the collateral0 and collateral1, which is computed from the liquidity amount and tick range

but if collateral0 and collateral1 are small,

(token0Premium + premium0) * Base.BASIS_POINT) / collateral0 can excceed uint24 and unsafe downcasting result in loss of precision

Tools Used

Manual Review

Recommended Mitigation Steps

use the same uint256ToUint24 in the function addPremium

Assessed type

Math

[c4-judge]

0xleastwood marked the issue as primary issue

[JeffCX]

duplicate of #43

[romeroadrian]

The uint24 holds the relation between the two tokens, which are denominated in the same decimal domain. The overflow would be caused if premium > (2**24 - 1) * collateral / 1e6, an impractical scenario.

[JeffCX]

Thanks for reviewing my submission @romeroadrian

I think #43 has a better POC

2 ** 24 - 1 is a very small number

2 ** 24 - 1 = 16777215

while if token has 18 decimals

10 ** 18 is already 1000000000000000000

so loss of precision can happen

[romeroadrian]

Thanks for reviewing my submission @romeroadrian

I think #43 has a better POC

2 ** 24 - 1 is a very small number

2 ** 24 - 1 = 16777215

while if token has 18 decimals

10 ** 18 is already 1000000000000000000

so loss of precision can happen

Both sides have the same decimals, the portion is storing a relation.

[JeffCX]

your comment is true,

but certain token has high total supply and low price

https://coinmarketcap.com/currencies/shiba-inu/

ok let us consider the token is shiba

price per shib is 0.00001069

adding 16777215 shib is a very small amount of money

16777215 * 0.00001069 = 179.2 USD

it is likely user can add 1000 USD worth+ shib, cause loss of precision

[romeroadrian]

16777215 is the relation, not the net amount. What if you put some numbers to premium and collateral and show how that overflows 24 bits?

[wukong-particle]

Thanks for raising the concern about large numbers in e.g. shib. I think @romeroadrian is right here that the denominator would also be large (in the same magnitude as the 16777215 input shib).

For this uint24 portion to be violated, premium > (2**24 - 1) * collateral / BASIS_POINT should happen, which means borrower is putting in 16 times more premium than the borrowed amount, should be unlikely for "leveraged" trading.

[wukong-particle]

Oh but I agree we should use uint256ToUint24 instead. Similar to #43.

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

wukong-particle (sponsor) disputed

[wukong-particle]

Putting in dispute validity for the nominator denominator ratio discussion (which is what this issue is mostly about), but if possible, I will put "sponsor confirmed" for the uint256ToUint24 discussion in the comment, similar to #43

[0xleastwood]

I'm going to downgrade this to QA because it requires some assumptions in the usage of certain tokens and significantly more premium than what is being borrowed.

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

0xleastwood marked the issue as not selected for report