search

code-423n4 / 2024-10-ronin-validation

0 stars 0 forks source link

If `feeProtocolNum` equals `feeProtocolDen`, it would mean 100% of the fees go to the protocol, which is likely unintended. #45

Open c4-bot-3 opened 1 month ago

c4-bot-3 commented 1 month ago

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/KatanaV3Pool.sol#L253-L267

Vulnerability details

Proof of Concept

The initialize function retrieves feeProtocolNum and feeProtocolDen from the factory contract using IKatanaV3Factory(factory).feeAmountProtocol(fee). It then directly assigns these values to slot0.feeProtocolNum and slot0.feeProtocolDen without checking if feeProtocolNum < feeProtocolDen. This oversight in validation allows the pool to be initialized with a fee protocol ratio where feeProtocolNum could be greater than or equal to feeProtocolDen.

KatanaV3Pool.sol#L253-L262

/// @dev not locked because it initializes unlocked
function initialize(uint160 sqrtPriceX96) external override {
  require(slot0.sqrtPriceX96 == 0, "AI");

  int24 tick = TickMath.getTickAtSqrtRatio(sqrtPriceX96);

  (uint16 cardinality, uint16 cardinalityNext) = observations.initialize(_blockTimestamp());

  (uint8 feeProtocolNum, uint8 feeProtocolDen) = IKatanaV3Factory(factory).feeAmountProtocol(fee); // <= FOUND

  slot0 = Slot0({
    sqrtPriceX96: sqrtPriceX96,
    tick: tick,
    observationIndex: 0,
    observationCardinality: cardinality,
    observationCardinalityNext: cardinalityNext,
    feeProtocolNum: feeProtocolNum, // <= FOUND
    feeProtocolDen: feeProtocolDen, // <= FOUND
    unlocked: true
  });

  emit Initialize(sqrtPriceX96, tick);
}

Users may end up paying higher fees than expected or the protocol may collect more fees than intended. If a pool is initialized with an invalid fee protocol ratio (i.e., feeProtocolNum >= feeProtocolDen), it could lead to incorrect fee calculations and distributions.

Recommended Mitigation Steps

Add a check in the initialize function to ensure feeProtocolNum < feeProtocolDen before setting the values in slot0. Consider adding validation in the factory's feeAmountProtocol function to always return values satisfying this condition.

Assessed type

Invalid Validation

nevillehuang commented 3 weeks ago

Note, this are not present in the automated finding report for known issues Grouping all input validation issues together. They should all likely be at most QA and argubly invalid based on the following READ.ME information:

Centralization risk. Sky Mavis is responsible for maintaining the Katana V3 contracts and will able to upgrade the contract if necessary, as well as specify additional fee tiers.