Authorization function of the Governance can still restrict user from migrating v2 liquidity to v3 liquidity

[howlbot-integration]

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/periphery/V3Migrator.sol#L36

Vulnerability details

Proof of concept

The readme clearly say that once the pool is created and initialized,

the governance should not be able to block user from migrating their V2 position to V3.

If a liquidity pool (pair of tokens) is already open for liquidity provision on Katana V2, liquidity providers are expected to be able to migrate their liquidity to the corresponding pool on Katana V3 when it is created, without being restricted by the authorization function of the Governance.

Howver, this is not the case.

When migrating, this is the code:

  function migrate(MigrateParams calldata params) external override {
    require(params.percentageToMigrate > 0, "Percentage too small");
    require(params.percentageToMigrate <= 100, "Percentage too large");

    // burn v2 liquidity to this address
    TransferHelper.safeTransferFrom(params.pair, msg.sender, params.pair, params.liquidityToMigrate);
    (uint256 amount0V2, uint256 amount1V2) = IKatanaV2Pair(params.pair).burn(address(this));

    // calculate the amounts to migrate to v3
    uint256 amount0V2ToMigrate = amount0V2.mul(params.percentageToMigrate) / 100;
    uint256 amount1V2ToMigrate = amount1V2.mul(params.percentageToMigrate) / 100;

    // approve the position manager up to the maximum token amounts
    TransferHelper.safeApprove(params.token0, nonfungiblePositionManager, amount0V2ToMigrate);
    TransferHelper.safeApprove(params.token1, nonfungiblePositionManager, amount1V2ToMigrate);

    // mint v3 position
    (,, uint256 amount0V3, uint256 amount1V3) = INonfungiblePositionManager(nonfungiblePositionManager).mint(
      INonfungiblePositionManager.MintParams({
        token0: params.token0,
        token1: params.token1,
        fee: params.fee,
        tickLower: params.tickLower,
        tickUpper: params.tickUpper,
        amount0Desired: amount0V2ToMigrate,
        amount1Desired: amount1V2ToMigrate,
        amount0Min: params.amount0Min,
        amount1Min: params.amount1Min,
        recipient: params.recipient,
        deadline: params.deadline
      })
    );

then the code hits the NonfungiblePositionManager.sol#mint

  /// @inheritdoc INonfungiblePositionManager
  function mint(MintParams calldata params)
    external
    payable
    override
    checkDeadline(params.deadline)
    returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
  {
    AuthorizationLib.checkPair(governance, params.token0, params.token1);

this is calling checkPair

  function checkPair(address governance, address token0, address token1) internal view {
    address[] memory tokens = new address[](2);
    tokens[0] = token0;
    tokens[1] = token1;
    require(IKatanaGovernance(governance).isAuthorized(tokens, msg.sender), "UA");
  }

the governance code is:

  function isAuthorized(address token, address account) public view returns (bool authorized) {
    if (_isSkipped(account)) return true;

    authorized = _isAuthorized(_permission[token], account);
  }

  /**
   * @inheritdoc IKatanaGovernance
   */
  function isAuthorized(address[] memory tokens, address account) public view returns (bool authorized) {
    if (_isSkipped(account)) return true;

    uint256 length = tokens.length;
    for (uint256 i; i < length; ++i) {
      if (!_isAuthorized(_permission[tokens[i]], account)) return false;
    }

    return true;
  }

After the pool is created, the admin can still call the setPermission to block user from migrating the certain token's v2 position to v3.

  function setPermission(address token, uint40 whitelistUntil, address[] calldata alloweds, bool[] calldata statuses)
    external
    onlyOwner
  {
    _setPermission(token, whitelistUntil, alloweds, statuses);
  }

Recommendation

Strictly enforce that:

If a liquidity pool (pair of tokens) is already open for liquidity provision on Katana V2, liquidity providers are expected to be able to migrate their liquidity to the corresponding pool on Katana V3 when it is created, without being restricted by the authorization function of the Governance.

Assessed type

Access Control

[thaixuandang]

V3Migrator is an allowedActor (this is pre-set in our script, see initializeV2 function and script), meaning the isAuthorized function will always return true when the account passed in is V3Migrator. In the migrate logic above, when entering the checkPair function, msg.sender here is V3Migrator, so the isAuthorized function will always return true.

[alex-ppg]

The Warden attempts to breach an invariant of the system in relation to liquidity migrations due to improper enforcement of governance restrictions in the V3Migrator::migrate function.

This particular submission is invalid given that the msg.sender validated as having been approved by the governance is the V3Migrator itself which is expected (and validated through scripts) to be approved when it is deployed.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[JeffCX]

meaning the isAuthorized function will always return true when the account passed in is V3Migrator. In the migrate logic above, when entering the checkPair function, msg.sender here is V3Migrator, so the isAuthorized function will always return true.

this is true, but this report says

After the pool is created, the admin can still call the setPermission to block user from migrating the certain token's v2 position to v3.

 function setPermission(address token, uint40 whitelistUntil, address[] calldata alloweds, bool[] calldata statuses)
    external
    onlyOwner
  {
    _setPermission(token, whitelistUntil, alloweds, statuses);
  }

the doc says:

If a liquidity pool (pair of tokens) is already open for liquidity provision on Katana V2, liquidity providers are expected to be able to migrate their liquidity to the corresponding pool on Katana V3 when it is created, without being restricted by the authorization function of the Governance.

I read it as migration cannot fail after the pair is created,

but let us say the RONIN / ETH pair is created,

the admin remove the permission of ETH token,

migrator call mint

  /// @inheritdoc INonfungiblePositionManager
  function mint(MintParams calldata params)
    external
    payable
    override
    checkDeadline(params.deadline)
    returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
  {
    AuthorizationLib.checkPair(governance, params.token0, params.token1);

and. the code check if each token is [authorized[(https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/external/libraries/AuthorizationLib.sol#L7)

note the check:

  function isAuthorized(address token, address account) public view returns (bool authorized) {
    if (_isSkipped(account)) return true;

    authorized = _isAuthorized(_permission[token], account);
  }

  /**
   * @inheritdoc IKatanaGovernance
   */
  function isAuthorized(address[] memory tokens, address account) public view returns (bool authorized) {
    if (_isSkipped(account)) return true;

    uint256 length = tokens.length;
    for (uint256 i; i < length; ++i) {
      if (!_isAuthorized(_permission[tokens[i]], account)) return false;
    }

    return true;
  }

as for the comments:

meaning the isAuthorized function will always return true when the account passed in is V3Migrator

the admin can first call setAllowedActor to make _isSkipped return false,

then call the setPermission to block user from migrating the certain token's v2 position to v3.

which violate the statement:

If a liquidity pool (pair of tokens) is already open for liquidity provision on Katana V2, liquidity providers are expected to be able to migrate their liquidity to the corresponding pool on Katana V3 when it is created, without being restricted by the authorization function of the Governance.

I politely think this issue should be a medium or grade a QA. thank you!.

[alex-ppg]

Hey @JeffCX, thanks for your PJQA contribution! Arguing for a medium-severity rating for this submission cannot be done in good faith in my opinion as it clearly demonstrates a would-be issue of negligible impact. While I can see how this could be argued as a QA severity issue, I still do not believe it constitutes an HM vulnerability and will retain an invalidation.

[JeffCX]

https://code4rena.com/audits/2024-10-ronin

this is the assumption in the doc page that should not be taken for granted.

If a liquidity pool (pair of tokens) is already open for liquidity provision on Katana V2, liquidity providers are expected to be able to migrate their liquidity to the corresponding pool on Katana V3 when it is created, without being restricted by the authorization function of the Governance.

If this assumption is broken, I think this QA should get some credit instead of invalidation.