Validate `poolIdx` input to prevent storage corruption in critical functions.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L156-L168

Vulnerability details

Impact

No validation on poolIdx input for key functions like claimConcentratedRewards. Could pass invalid poolId and corrupt storage.

Proof of Concept

The claimConcentratedRewards function.

It takes in a poolIdx as one of the parameters:

function claimConcentratedRewards(
  address payable owner,
  bytes32 poolIdx,  
  int24 lowerTick,
  int24 upperTick,
  uint32[] memory weeksToClaim
) internal {

  // Reward claiming logic  

}

However, there is no validation on the poolIdx input anywhere in the function.

The lack of validation on this input parameter is a security risk.

This could allow an invalid poolId to be passed in, resulting in errors or storage corruption. For example:

claimConcentratedRewards(
  owner, 
  0x123456789, // Invalid poolId
  -60, 
  60,
  [1] 
);

Here is a more in-depth explanation of this issue.

function claimConcentratedRewards(
  address payable owner,
  bytes32 poolIdx, // Pool identifier 
  // other params
) internal {

  // Claim reward logic

}

As can be seen, the problem is there is no validation on the poolIdx input anywhere in the function.

This means any arbitrary 32 byte value can be passed in as the pool identifier. For example:

claimConcentratedRewards(
  owner,
  0x123456789012345678901234567890123456, // Invalid poolId
  // other params  
);

Passing an invalid poolId like this can corrupt storage by:

• Writing reward tracking data to invalid array indexes
• Accruing time-weighted liquidity for non-existent pools
• Transferring funds to the wrong accounts

It violates the assumption that poolIdx maps to a valid pool.

Here is some example showing how an attacker could pass an invalid poolID to corrupt storage in claimConcentratedRewards():

contract Attacker {

  LiquidityMining public liquidityMining;

  // Construct with real liquidity mining address
  constructor(address _liquidityMiningAddress) {
    liquidityMining = LiquidityMining(_liquidityMiningAddress);
  }

  function exploitInvalidPoolId() public {
    bytes32 invalidPoolId = 0x123456789012345678901234567890123456;

    liquidityMining.claimConcentratedRewards{value: 0}(
      address(1),
      invalidPoolId,
      -60,
      60,
      [uint32(block.timestamp)]
    );
  }

}

This attacker contract calls claimConcentratedRewards() with an invalid poolID that does not map to any real pool.

Since there is no validation, this invalid ID will be used in the reward logic, corrupting storage:

• Invalid array indexes accessed in tickTracking_
• Time-weighted liquidity accrued for fake pool
• concLiquidityRewardsClaimed_ modified for non-existent pool

Over time this could seriously corrupt storage and accounting for the protocol.

Tools Used

Vs

Recommended Mitigation Steps

Adding a require statement like:

require(isValidPool(poolIdx), "Invalid pool");

Would prevent invalid poolIds from being used.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient quality