Authorisation check for minting positions for V3 pools in the nfpManager is pointing to the v2Factory

[c4-bot-8]

Lines of code

https://github.com/ronin-chain/katana-operation-contracts/blob/27f9d28e00958bf3494fa405a8a5acdcd5ecdc5d/src/governance/KatanaGovernance.sol#L332-L334

Vulnerability details

Impact

Part of function which checks, if a user is allowed to mint a position in a V3 pool through the nfpManager points to the v2Factory and not the v3Factory. This can lead to a situation where anyone is able to mint a position in a V3 pool even though the minting should be restricted. This renders the authorisation check in the nfpManager useless.

Proof of Concept

When a user wants to mint a position in the NonfungiblePositionManager, it is first checked if he has the authorisation to do so:

  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);
          …
}

The checkPair function calls the governance function isAuthorized which in turn call the _isSkipped function to check if the authorisation for the pool (based on the tokens) should be skipped:

  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;
  } 

  function _isSkipped(address account) internal view returns (bool) {
    return isAllowedActor[account] || allowedAll() || account == owner();
  }

The _isSkipped function returns true and thereby authorises the caller to mint if the account is

• an allowed actor (isAllowedActor)
• the owner or
• if all is allowed (allawedAll).

The issue arises from the fact that the user attempts to mint a position in a V3 pool but the allawedAll function checks the value of allawedAll in the v2 factory:

  function allowedAll() public view returns (bool) {
 @>   return _v2Factory.allowedAll();
  }

This means that even if the user is not allowed to mint a position for a V3 pool, if allowedAll in the v2Factory is set to true he will be able to do so rendering the authorisation functionality of the nfpManager useless.

Recommended Mitigation Steps

Consider adding another function to the governance contract for checking the authorisation for V3 pools which does not depend on the value of allowedAll in the _v2Factory

Assessed type

Access Control

[khangvv]

This issue is implemented by design to ensure that both Katana V2 and V3 align with the same authorization checks. Additionally, it preserves the existing governance feature from V2 to maintain consistency.

[alex-ppg]

The Warden claims that the V3 implementation of the system should implement a distinct allowed-all function implementation from the V2 system. There is no vulnerability that arises from this behavior, and the code does not indicate that a distinct system should be utilized which is in line with the Sponsor's intentions as expressed here.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid