Wrong implementation of accesss control for KatanaGovernance._authorized()

[howlbot-integration]

Lines of code

/root/katana-operation-contracts/src/governance/KatanaGovernance.sol#L377-L383

Vulnerability details

Proof of Concept

The following implementation of _isAuthorized is wrong since an account is considered authorized if
block.timestamp > expiry even if $.allowed[account] = false. Meanwhile, even after the authorization expires, if $.allowed[account] is true, then it is still authorized.

 function _isAuthorized(Permission storage $, address account) private view returns (bool) {
    uint256 expiry = $.whitelistUntil;
    if (expiry == UNAUTHORIZED) return false;
    if (expiry == AUTHORIZED || block.timestamp > expiry) return true; // but we have not checked the account!

    return $.allowed[account];
  }

Recommended Mitigation Steps

The correct way is to check both expiration time and $.allowed[account] with a conjunction.

 function _isAuthorized(Permission storage $, address account) private view returns (bool) {
    uint256 expiry = $.whitelistUntil;
    if (expiry == UNAUTHORIZED) return false;
-    if (expiry == AUTHORIZED || block.timestamp > expiry) return true; // but we have not checked the account!

+    if(expiry = AUTHORIZED) return true;

-    return $.allowed[account];
-    if(block.timestamp > expiry && $.allowed[account]) return true;

  }

Assessed type

Access Control

[khangvv]

The authorization checks are prioritized as written in the code. Both timestamp-based permissions and those allowed via the array are valid options and should be treated as granting access. This functions as a "merge" strategy for the whitelist rather than a strict conjunction.

[khanhtaskymaviscom]

This is a design decision, not a bug. The protocol initially allows certain accounts to interact with the token/pool for a set period. After this initial phase, anyone can interact with the created pool.

[alex-ppg]

The submission and its duplicates seem to have misinterpreted the whitelist system in use by the code. Specifically, the expiry variable represents the timestamp at which the whitelist should no longer be enforced and all users should have access.

As such, the code presently behaves correctly as it will either disallow access due to the special UNAUTHORIZED flag, allow access due to the special AUTHORIZED flag, or if the whitelist is no longer meant to be enforced, or allow access conditionally per account if the whitelist is in effect.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid