Wrong implementation of accesss control for KatanaGovernance._authorized()

[c4-bot-8]

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