Missing address(0) check in the Payments::pay function.

[c4-bot-9]

Lines of code

https://github.com/ronin-chain/katana-operation-contracts/blob/27f9d28e00958bf3494fa405a8a5acdcd5ecdc5d/src/aggregate-router/modules/Payments.sol#L26

Vulnerability details

Proof of Concept

In the Payments::pay function, the fund is sent to the address recipient but no validation has been done to check the contents of this input, so any address can be supplied, even the address address(0) which is an invalid address. Any funds sent to this address will be burnt and can never be recovered.

    function pay(address token, address recipient, uint256 value) internal {
    // @audit lack of address(0) check to recipient input
    if (token == Constants.ETH) {
      recipient.safeTransferETH(value);
    } else {
      if (value == Constants.CONTRACT_BALANCE) {
        value = ERC20(token).balanceOf(address(this));
      }

      ERC20(token).safeTransfer(recipient, value);
    }
  }

Impact

Loss of funds: all funds sent to address(0) will be burnt.

Recommended Mitigation Steps

    function pay(address token, address recipient, uint256 value) internal {
+   require(recipient != address(0), "Invalid recipient address");
    if (token == Constants.ETH) {
      recipient.safeTransferETH(value);
    } else {
      if (value == Constants.CONTRACT_BALANCE) {
        value = ERC20(token).balanceOf(address(this));
      }

      ERC20(token).safeTransfer(recipient, value);
    }
  }

Assessed type

Invalid Validation

[nevillehuang]

Note, this are not present in the automated finding report for known issues. Grouping all address(0) input validation checks 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.

#101 would require a user error.

• Missing address(0) while initializing the PeripheryImmutableState in his constructor. #111
• Lack of address(0) in the KatanaGovernance::_setFactory function #107
• Missing address(0) check in the Payments::pay function. #101
• Insufficient input validation may lead to loss of funds due to invalid or malicious input data. #183