From 2f8941c795d34c58e48202a676d42cd8c513770e Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Fri, 14 Feb 2025 12:36:24 -0500 Subject: [PATCH] docs: upgrades can invalidate permissions (#1096) **Motivation:** Audit report flagged that function selector-based permissions may break on upgrades. This PR documents the limitation and its implications while improving NatSpec for clarity. (EGSL-15) **Modifications:** - Documented function selector upgrade invalidations. - Improved NatSpec comments in `IPermissionController`. **Result:** Clearer documentation on function selector limitations and enhanced NatSpec for better code clarity. --- .../interfaces/IPermissionController.sol | 142 ++++++++++-------- 1 file changed, 78 insertions(+), 64 deletions(-) diff --git a/src/contracts/interfaces/IPermissionController.sol b/src/contracts/interfaces/IPermissionController.sol index 5d4be806d6..82940dafa1 100644 --- a/src/contracts/interfaces/IPermissionController.sol +++ b/src/contracts/interfaces/IPermissionController.sol @@ -2,141 +2,154 @@ pragma solidity ^0.8.27; interface IPermissionControllerErrors { - /// @notice Thrown when the caller is not the admin + /// @notice Thrown when a non-admin caller attempts to perform an admin-only action. error NotAdmin(); - /// @notice Thrown when the admin to remove is not an admin + /// @notice Thrown when attempting to remove an admin that does not exist. error AdminNotSet(); - /// @notice Thrown when an appointee is already set for the account's function + /// @notice Thrown when attempting to set an appointee for a function that already has one. error AppointeeAlreadySet(); - /// @notice Thrown when an appointee is not set for the account's function + /// @notice Thrown when attempting to interact with a non-existent appointee. error AppointeeNotSet(); - /// @notice Thrown when the account attempts to remove the only admin + /// @notice Thrown when attempting to remove the last remaining admin. error CannotHaveZeroAdmins(); - /// @notice Thrown when an admin is already set + /// @notice Thrown when attempting to set an admin that is already registered. error AdminAlreadySet(); - /// @notice Thrown when an admin is not pending + /// @notice Thrown when attempting to interact with an admin that is not in pending status. error AdminNotPending(); - /// @notice Thrown when an admin is already pending + /// @notice Thrown when attempting to add an admin that is already pending. error AdminAlreadyPending(); } interface IPermissionControllerEvents { - /// @notice Emitted when an appointee is set + /// @notice Emitted when an appointee is set for an account to handle specific function calls. event AppointeeSet(address indexed account, address indexed appointee, address target, bytes4 selector); - /// @notice Emitted when an appointee is revoked + /// @notice Emitted when an appointee's permission to handle function calls for an account is revoked. event AppointeeRemoved(address indexed account, address indexed appointee, address target, bytes4 selector); - /// @notice Emitted when an admin is set as pending for an account + /// @notice Emitted when an address is set as a pending admin for an account, requiring acceptance. event PendingAdminAdded(address indexed account, address admin); - /// @notice Emitted when an admin is removed as pending for an account + /// @notice Emitted when a pending admin status is removed for an account before acceptance. event PendingAdminRemoved(address indexed account, address admin); - /// @notice Emitted when an admin is set for a given account + /// @notice Emitted when an address accepts and becomes an active admin for an account. event AdminSet(address indexed account, address admin); - /// @notice Emitted when an admin is removed for a given account + /// @notice Emitted when an admin's permissions are removed from an account. event AdminRemoved(address indexed account, address admin); } interface IPermissionController is IPermissionControllerErrors, IPermissionControllerEvents { /** - * @notice Sets a pending admin of an account - * @param account to set pending admin for - * @param admin to set - * @dev Multiple admins can be set for an account + * @notice Sets a pending admin for an account. + * @param account The account to set the pending admin for. + * @param admin The address to set as pending admin. + * @dev The pending admin must accept the role before becoming an active admin. + * @dev Multiple admins can be set for a single account. */ function addPendingAdmin(address account, address admin) external; /** - * @notice Removes a pending admin of an account - * @param account to remove pending admin for - * @param admin to remove - * @dev Only the admin of the account can remove a pending admin + * @notice Removes a pending admin from an account before they have accepted the role. + * @param account The account to remove the pending admin from. + * @param admin The pending admin address to remove. + * @dev Only an existing admin of the account can remove a pending admin. */ function removePendingAdmin(address account, address admin) external; /** - * @notice Accepts the admin role of an account - * @param account to accept admin for - * @dev Only a pending admin for the account can become an admin + * @notice Allows a pending admin to accept their admin role for an account. + * @param account The account to accept the admin role for. + * @dev Only addresses that were previously set as pending admins can accept the role. */ function acceptAdmin( address account ) external; /** - * @notice Remove an admin of an account - * @param account to remove admin for - * @param admin to remove - * @dev Only the admin of the account can remove an admin - * @dev Reverts when an admin is removed such that no admins are remaining + * @notice Removes an active admin from an account. + * @param account The account to remove the admin from. + * @param admin The admin address to remove. + * @dev Only an existing admin of the account can remove another admin. + * @dev Will revert if removing this admin would leave the account with zero admins. */ function removeAdmin(address account, address admin) external; /** - * @notice Set an appointee for a given account - * @param account to set appointee for - * @param appointee to set - * @param target to set appointee for - * @param selector to set appointee for - * @dev Only the admin of the account can set an appointee + * @notice Sets an appointee who can call specific functions on behalf of an account. + * @param account The account to set the appointee for. + * @param appointee The address to be given permission. + * @param target The contract address the appointee can interact with. + * @param selector The function selector the appointee can call. + * @dev Only an admin of the account can set appointees. */ function setAppointee(address account, address appointee, address target, bytes4 selector) external; /** - * Removes an appointee for a given account - * @param account to remove appointee for - * @param appointee to remove - * @param target to remove appointee for - * @param selector to remove appointee for - * @dev Only the admin of the account can remove an appointee + * @notice Removes an appointee's permission to call a specific function. + * @param account The account to remove the appointee from. + * @param appointee The appointee address to remove. + * @param target The contract address to remove permissions for. + * @param selector The function selector to remove permissions for. + * @dev Only an admin of the account can remove appointees. */ function removeAppointee(address account, address appointee, address target, bytes4 selector) external; /** - * @notice Checks if the given caller is an admin of the account - * @dev If the account has no admin, the caller is checked to be the account itself + * @notice Checks if a given address is an admin of an account. + * @param account The account to check admin status for. + * @param caller The address to check. + * @dev If the account has no admins, returns true only if the caller is the account itself. + * @return Returns true if the caller is an admin, false otherwise. */ function isAdmin(address account, address caller) external view returns (bool); /** - * @notice Checks if the `pendingAdmin` is a pending admin of the `account` + * @notice Checks if an address is currently a pending admin for an account. + * @param account The account to check pending admin status for. + * @param pendingAdmin The address to check. + * @return Returns true if the address is a pending admin, false otherwise. */ function isPendingAdmin(address account, address pendingAdmin) external view returns (bool); /** - * @notice Get the admins of an account - * @param account The account to get the admin of - * @dev If the account has no admin, the account itself is returned + * @notice Retrieves all active admins for an account. + * @param account The account to get the admins for. + * @dev If the account has no admins, returns an array containing only the account address. + * @return An array of admin addresses. */ function getAdmins( address account ) external view returns (address[] memory); /** - * @notice Get the pending admins of an account - * @param account The account to get the pending admin of + * @notice Retrieves all pending admins for an account. + * @param account The account to get the pending admins for. + * @return An array of pending admin addresses. */ function getPendingAdmins( address account ) external view returns (address[] memory); /** - * @notice Checks if the given caller has permissions to call the fucntion - * @param account to check - * @param caller to check permission for - * @param target to check permission for - * @param selector to check permission for - * @dev Returns `true` if the admin OR the appointee is the caller + * @notice Checks if a caller has permission to call a specific function. + * @param account The account to check permissions for. + * @param caller The address attempting to make the call. + * @param target The contract address being called. + * @param selector The function selector being called. + * @dev Returns true if the caller is either an admin or an appointed caller. + * @dev Be mindful that upgrades to the contract may invalidate the appointee's permissions. + * This is only possible if a function's selector changes (e.g. if a function's parameters are modified). + * @return Returns true if the caller has permission, false otherwise. */ function canCall(address account, address caller, address target, bytes4 selector) external returns (bool); /** - * @notice Gets the list of permissions of an appointee for a given account - * @param account to get appointee permissions for - * @param appointee to get permissions + * @notice Retrieves all permissions granted to an appointee for a given account. + * @param account The account to check appointee permissions for. + * @param appointee The appointee address to check. + * @return Two arrays: target contract addresses and their corresponding function selectors. */ function getAppointeePermissions( address account, @@ -144,11 +157,12 @@ interface IPermissionController is IPermissionControllerErrors, IPermissionContr ) external returns (address[] memory, bytes4[] memory); /** - * @notice Returns the list of appointees for a given account and function - * @param account to get appointees for - * @param target to get appointees for - * @param selector to get appointees for - * @dev Does NOT include admin as an appointee, even though it can call + * @notice Retrieves all appointees that can call a specific function for an account. + * @param account The account to get appointees for. + * @param target The contract address to check. + * @param selector The function selector to check. + * @dev Does not include admins in the returned list, even though they have calling permission. + * @return An array of appointee addresses. */ function getAppointees(address account, address target, bytes4 selector) external returns (address[] memory); }