Skip to content

Commit 41e17da

Browse files
0xClandestineypatil12
authored andcommitted
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.
1 parent 74d5fd7 commit 41e17da

File tree

1 file changed

+78
-64
lines changed

1 file changed

+78
-64
lines changed

src/contracts/interfaces/IPermissionController.sol

Lines changed: 78 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -2,153 +2,167 @@
22
pragma solidity ^0.8.27;
33

44
interface IPermissionControllerErrors {
5-
/// @notice Thrown when the caller is not the admin
5+
/// @notice Thrown when a non-admin caller attempts to perform an admin-only action.
66
error NotAdmin();
7-
/// @notice Thrown when the admin to remove is not an admin
7+
/// @notice Thrown when attempting to remove an admin that does not exist.
88
error AdminNotSet();
9-
/// @notice Thrown when an appointee is already set for the account's function
9+
/// @notice Thrown when attempting to set an appointee for a function that already has one.
1010
error AppointeeAlreadySet();
11-
/// @notice Thrown when an appointee is not set for the account's function
11+
/// @notice Thrown when attempting to interact with a non-existent appointee.
1212
error AppointeeNotSet();
13-
/// @notice Thrown when the account attempts to remove the only admin
13+
/// @notice Thrown when attempting to remove the last remaining admin.
1414
error CannotHaveZeroAdmins();
15-
/// @notice Thrown when an admin is already set
15+
/// @notice Thrown when attempting to set an admin that is already registered.
1616
error AdminAlreadySet();
17-
/// @notice Thrown when an admin is not pending
17+
/// @notice Thrown when attempting to interact with an admin that is not in pending status.
1818
error AdminNotPending();
19-
/// @notice Thrown when an admin is already pending
19+
/// @notice Thrown when attempting to add an admin that is already pending.
2020
error AdminAlreadyPending();
2121
}
2222

2323
interface IPermissionControllerEvents {
24-
/// @notice Emitted when an appointee is set
24+
/// @notice Emitted when an appointee is set for an account to handle specific function calls.
2525
event AppointeeSet(address indexed account, address indexed appointee, address target, bytes4 selector);
2626

27-
/// @notice Emitted when an appointee is revoked
27+
/// @notice Emitted when an appointee's permission to handle function calls for an account is revoked.
2828
event AppointeeRemoved(address indexed account, address indexed appointee, address target, bytes4 selector);
2929

30-
/// @notice Emitted when an admin is set as pending for an account
30+
/// @notice Emitted when an address is set as a pending admin for an account, requiring acceptance.
3131
event PendingAdminAdded(address indexed account, address admin);
3232

33-
/// @notice Emitted when an admin is removed as pending for an account
33+
/// @notice Emitted when a pending admin status is removed for an account before acceptance.
3434
event PendingAdminRemoved(address indexed account, address admin);
3535

36-
/// @notice Emitted when an admin is set for a given account
36+
/// @notice Emitted when an address accepts and becomes an active admin for an account.
3737
event AdminSet(address indexed account, address admin);
3838

39-
/// @notice Emitted when an admin is removed for a given account
39+
/// @notice Emitted when an admin's permissions are removed from an account.
4040
event AdminRemoved(address indexed account, address admin);
4141
}
4242

4343
interface IPermissionController is IPermissionControllerErrors, IPermissionControllerEvents {
4444
/**
45-
* @notice Sets a pending admin of an account
46-
* @param account to set pending admin for
47-
* @param admin to set
48-
* @dev Multiple admins can be set for an account
45+
* @notice Sets a pending admin for an account.
46+
* @param account The account to set the pending admin for.
47+
* @param admin The address to set as pending admin.
48+
* @dev The pending admin must accept the role before becoming an active admin.
49+
* @dev Multiple admins can be set for a single account.
4950
*/
5051
function addPendingAdmin(address account, address admin) external;
5152

5253
/**
53-
* @notice Removes a pending admin of an account
54-
* @param account to remove pending admin for
55-
* @param admin to remove
56-
* @dev Only the admin of the account can remove a pending admin
54+
* @notice Removes a pending admin from an account before they have accepted the role.
55+
* @param account The account to remove the pending admin from.
56+
* @param admin The pending admin address to remove.
57+
* @dev Only an existing admin of the account can remove a pending admin.
5758
*/
5859
function removePendingAdmin(address account, address admin) external;
5960

6061
/**
61-
* @notice Accepts the admin role of an account
62-
* @param account to accept admin for
63-
* @dev Only a pending admin for the account can become an admin
62+
* @notice Allows a pending admin to accept their admin role for an account.
63+
* @param account The account to accept the admin role for.
64+
* @dev Only addresses that were previously set as pending admins can accept the role.
6465
*/
6566
function acceptAdmin(
6667
address account
6768
) external;
6869

6970
/**
70-
* @notice Remove an admin of an account
71-
* @param account to remove admin for
72-
* @param admin to remove
73-
* @dev Only the admin of the account can remove an admin
74-
* @dev Reverts when an admin is removed such that no admins are remaining
71+
* @notice Removes an active admin from an account.
72+
* @param account The account to remove the admin from.
73+
* @param admin The admin address to remove.
74+
* @dev Only an existing admin of the account can remove another admin.
75+
* @dev Will revert if removing this admin would leave the account with zero admins.
7576
*/
7677
function removeAdmin(address account, address admin) external;
7778

7879
/**
79-
* @notice Set an appointee for a given account
80-
* @param account to set appointee for
81-
* @param appointee to set
82-
* @param target to set appointee for
83-
* @param selector to set appointee for
84-
* @dev Only the admin of the account can set an appointee
80+
* @notice Sets an appointee who can call specific functions on behalf of an account.
81+
* @param account The account to set the appointee for.
82+
* @param appointee The address to be given permission.
83+
* @param target The contract address the appointee can interact with.
84+
* @param selector The function selector the appointee can call.
85+
* @dev Only an admin of the account can set appointees.
8586
*/
8687
function setAppointee(address account, address appointee, address target, bytes4 selector) external;
8788

8889
/**
89-
* Removes an appointee for a given account
90-
* @param account to remove appointee for
91-
* @param appointee to remove
92-
* @param target to remove appointee for
93-
* @param selector to remove appointee for
94-
* @dev Only the admin of the account can remove an appointee
90+
* @notice Removes an appointee's permission to call a specific function.
91+
* @param account The account to remove the appointee from.
92+
* @param appointee The appointee address to remove.
93+
* @param target The contract address to remove permissions for.
94+
* @param selector The function selector to remove permissions for.
95+
* @dev Only an admin of the account can remove appointees.
9596
*/
9697
function removeAppointee(address account, address appointee, address target, bytes4 selector) external;
9798

9899
/**
99-
* @notice Checks if the given caller is an admin of the account
100-
* @dev If the account has no admin, the caller is checked to be the account itself
100+
* @notice Checks if a given address is an admin of an account.
101+
* @param account The account to check admin status for.
102+
* @param caller The address to check.
103+
* @dev If the account has no admins, returns true only if the caller is the account itself.
104+
* @return Returns true if the caller is an admin, false otherwise.
101105
*/
102106
function isAdmin(address account, address caller) external view returns (bool);
103107

104108
/**
105-
* @notice Checks if the `pendingAdmin` is a pending admin of the `account`
109+
* @notice Checks if an address is currently a pending admin for an account.
110+
* @param account The account to check pending admin status for.
111+
* @param pendingAdmin The address to check.
112+
* @return Returns true if the address is a pending admin, false otherwise.
106113
*/
107114
function isPendingAdmin(address account, address pendingAdmin) external view returns (bool);
108115

109116
/**
110-
* @notice Get the admins of an account
111-
* @param account The account to get the admin of
112-
* @dev If the account has no admin, the account itself is returned
117+
* @notice Retrieves all active admins for an account.
118+
* @param account The account to get the admins for.
119+
* @dev If the account has no admins, returns an array containing only the account address.
120+
* @return An array of admin addresses.
113121
*/
114122
function getAdmins(
115123
address account
116124
) external view returns (address[] memory);
117125

118126
/**
119-
* @notice Get the pending admins of an account
120-
* @param account The account to get the pending admin of
127+
* @notice Retrieves all pending admins for an account.
128+
* @param account The account to get the pending admins for.
129+
* @return An array of pending admin addresses.
121130
*/
122131
function getPendingAdmins(
123132
address account
124133
) external view returns (address[] memory);
125134

126135
/**
127-
* @notice Checks if the given caller has permissions to call the fucntion
128-
* @param account to check
129-
* @param caller to check permission for
130-
* @param target to check permission for
131-
* @param selector to check permission for
132-
* @dev Returns `true` if the admin OR the appointee is the caller
136+
* @notice Checks if a caller has permission to call a specific function.
137+
* @param account The account to check permissions for.
138+
* @param caller The address attempting to make the call.
139+
* @param target The contract address being called.
140+
* @param selector The function selector being called.
141+
* @dev Returns true if the caller is either an admin or an appointed caller.
142+
* @dev Be mindful that upgrades to the contract may invalidate the appointee's permissions.
143+
* This is only possible if a function's selector changes (e.g. if a function's parameters are modified).
144+
* @return Returns true if the caller has permission, false otherwise.
133145
*/
134146
function canCall(address account, address caller, address target, bytes4 selector) external returns (bool);
135147

136148
/**
137-
* @notice Gets the list of permissions of an appointee for a given account
138-
* @param account to get appointee permissions for
139-
* @param appointee to get permissions
149+
* @notice Retrieves all permissions granted to an appointee for a given account.
150+
* @param account The account to check appointee permissions for.
151+
* @param appointee The appointee address to check.
152+
* @return Two arrays: target contract addresses and their corresponding function selectors.
140153
*/
141154
function getAppointeePermissions(
142155
address account,
143156
address appointee
144157
) external returns (address[] memory, bytes4[] memory);
145158

146159
/**
147-
* @notice Returns the list of appointees for a given account and function
148-
* @param account to get appointees for
149-
* @param target to get appointees for
150-
* @param selector to get appointees for
151-
* @dev Does NOT include admin as an appointee, even though it can call
160+
* @notice Retrieves all appointees that can call a specific function for an account.
161+
* @param account The account to get appointees for.
162+
* @param target The contract address to check.
163+
* @param selector The function selector to check.
164+
* @dev Does not include admins in the returned list, even though they have calling permission.
165+
* @return An array of appointee addresses.
152166
*/
153167
function getAppointees(address account, address target, bytes4 selector) external returns (address[] memory);
154168
}

0 commit comments

Comments
 (0)