From 6d3047795045c3a73b4ed3c2e943ec77d3641303 Mon Sep 17 00:00:00 2001 From: The3D Date: Fri, 25 Jun 2021 11:10:01 +0200 Subject: [PATCH] feat:store permissionAdmin per user --- contracts/interfaces/IPermissionManager.sol | 65 +++++----- .../configuration/PermissionManager.sol | 113 ++++++++++++------ package-lock.json | 2 +- .../test-aave/permission-manager.spec.ts | 99 +++++++++------ 4 files changed, 178 insertions(+), 101 deletions(-) diff --git a/contracts/interfaces/IPermissionManager.sol b/contracts/interfaces/IPermissionManager.sol index 5c3613f8..6e983baa 100644 --- a/contracts/interfaces/IPermissionManager.sol +++ b/contracts/interfaces/IPermissionManager.sol @@ -1,21 +1,20 @@ pragma solidity 0.6.12; interface IPermissionManager { - - event RoleSet(address indexed user, uint256 indexed role, bool set); + event RoleSet(address indexed user, uint256 indexed role, address indexed whiteLister, bool set); event PermissionsAdminSet(address indexed user, bool set); /** * @dev Allows owner to add new permission admins - * @param users The addresses of the users to promote to permission admin + * @param admins The addresses to promote to permission admin **/ - function addPermissionAdmins(address[] calldata users) external; + function addPermissionAdmins(address[] calldata admins) external; /** * @dev Allows owner to remove permission admins - * @param users The addresses of the users to demote as permission admin + * @param admins The addresses to demote as permission admin **/ - function removePermissionAdmins(address[] calldata users) external; + function removePermissionAdmins(address[] calldata admins) external; /** * @dev Allows owner to whitelist a set of addresses for multiple roles @@ -32,39 +31,45 @@ interface IPermissionManager { function removePermissions(uint256[] calldata roles, address[] calldata users) external; /** - * @dev Returns the permissions configuration for a specific account - * @param account The address of the user - * @return the set of permissions states for the account + * @dev Returns the permissions configuration for a specific user + * @param user The address of the user + * @return the set of permissions states for the user **/ - function getAccountPermissions(address account) external view returns (uint256[] memory, uint256); + function getUserPermissions(address user) external view returns (uint256[] memory, uint256); /** - * @dev Used to query if a certain account has a certain role - * @param account The address of the user - * @return True if the account is in the specific role + * @dev Used to query if a certain user has a certain role + * @param user The address of the user + * @return True if the user is in the specific role **/ - function isInRole(address account, uint256 role) external view returns (bool); + function isInRole(address user, uint256 role) external view returns (bool); /** - * @dev Used to query if a certain account has the permissions admin role - * @param account The address of the user - * @return True if the account is a permissions admin, false otherwise + * @dev Used to query if a certain user has the permissions admin role + * @param user The address of the user + * @return True if the user is a permissions admin, false otherwise **/ - function isPermissionsAdmin(address account) external view returns (bool); + function isPermissionsAdmin(address user) external view returns (bool); - - /** - * @dev Used to query if a certain account satisfies certain roles - * @param account The address of the user + /** + * @dev Used to query if a certain user satisfies certain roles + * @param user The address of the user * @param roles The roles to check - * @return True if the account has all the roles, false otherwise + * @return True if the user has all the roles, false otherwise **/ - function isInAllRoles(address account, uint256[] calldata roles) external view returns (bool); - - /** - * @dev Used to query if a certain account is in at least one of the roles specified - * @param account The address of the user - * @return True if the account has all the roles, false otherwise + function isInAllRoles(address user, uint256[] calldata roles) external view returns (bool); + + /** + * @dev Used to query if a certain user is in at least one of the roles specified + * @param user The address of the user + * @return True if the user has all the roles, false otherwise **/ - function isInAnyRole(address account, uint256[] calldata roles) external view returns (bool); + function isInAnyRole(address user, uint256[] calldata roles) external view returns (bool); + + /** + * @dev Used to query if a certain user is in at least one of the roles specified + * @param user The address of the user + * @return the address of the permissionAdmin of the user + **/ + function getUserPermissionAdmin(address user) external view returns (address); } diff --git a/contracts/protocol/configuration/PermissionManager.sol b/contracts/protocol/configuration/PermissionManager.sol index c4b90e19..4a3156ad 100644 --- a/contracts/protocol/configuration/PermissionManager.sol +++ b/contracts/protocol/configuration/PermissionManager.sol @@ -10,7 +10,12 @@ import {Ownable} from '../../dependencies/openzeppelin/contracts/Ownable.sol'; * @author Aave **/ contract PermissionManager is IPermissionManager, Ownable { - mapping(address => uint256) _permissions; + struct UserData { + uint256 permissions; + address permissionAdmin; + } + + mapping(address => UserData) _users; mapping(address => uint256) _permissionsAdmins; uint256 public constant MAX_NUM_OF_ROLES = 256; @@ -21,27 +26,24 @@ contract PermissionManager is IPermissionManager, Ownable { } ///@inheritdoc IPermissionManager + function addPermissionAdmins(address[] calldata admins) external override onlyOwner { + for (uint256 i = 0; i < admins.length; i++) { + _permissionsAdmins[admins[i]] = 1; - function addPermissionAdmins(address[] calldata users) external override onlyOwner { - for (uint256 i = 0; i < users.length; i++) { - _permissionsAdmins[users[i]] = 1; - - emit PermissionsAdminSet(users[i], true); + emit PermissionsAdminSet(admins[i], true); } } ///@inheritdoc IPermissionManager + function removePermissionAdmins(address[] calldata admins) external override onlyOwner { + for (uint256 i = 0; i < admins.length; i++) { + _permissionsAdmins[admins[i]] = 0; - function removePermissionAdmins(address[] calldata users) external override onlyOwner { - for (uint256 i = 0; i < users.length; i++) { - _permissionsAdmins[users[i]] = 0; - - emit PermissionsAdminSet(users[i], false); + emit PermissionsAdminSet(admins[i], false); } } ///@inheritdoc IPermissionManager - function addPermissions(uint256[] calldata roles, address[] calldata users) external override @@ -51,18 +53,30 @@ contract PermissionManager is IPermissionManager, Ownable { for (uint256 i = 0; i < users.length; i++) { uint256 role = roles[i]; + address user = users[i]; require(role < MAX_NUM_OF_ROLES, 'INVALID_ROLE'); - uint256 permissions = _permissions[users[i]]; - _permissions[users[i]] = permissions | (1 << role); + uint256 permissions = _users[user].permissions; + address permissionAdmin = _users[user].permissionAdmin; - emit RoleSet(users[i], roles[i], true); + require( + (permissions != 0 && permissionAdmin == msg.sender) || + _users[user].permissionAdmin == address(0), + 'INVALID_PERMISSIONADMIN' + ); + + if (permissions == 0) { + _users[user].permissionAdmin = msg.sender; + } + + _users[user].permissions = permissions | (1 << role); + + emit RoleSet(user, role, msg.sender, true); } } ///@inheritdoc IPermissionManager - function removePermissions(uint256[] calldata roles, address[] calldata users) external override @@ -72,18 +86,32 @@ contract PermissionManager is IPermissionManager, Ownable { for (uint256 i = 0; i < users.length; i++) { uint256 role = roles[i]; + address user = users[i]; require(role < MAX_NUM_OF_ROLES, 'INVALID_ROLE'); - uint256 permissions = _permissions[users[i]]; - _permissions[users[i]] = permissions & ~(1 << role); - emit RoleSet(users[i], roles[i], false); + uint256 permissions = _users[user].permissions; + address permissionAdmin = _users[user].permissionAdmin; + + require( + (permissions != 0 && permissionAdmin == msg.sender) || + _users[user].permissionAdmin == address(0), + 'INVALID_PERMISSIONADMIN' + ); + + _users[user].permissions = permissions & ~(1 << role); + + if (_users[user].permissions == 0) { + //all permission have been removed + _users[user].permissionAdmin = address(0); + } + + emit RoleSet(user, role, msg.sender, false); } } ///@inheritdoc IPermissionManager - - function getAccountPermissions(address account) + function getUserPermissions(address user) external view override @@ -91,10 +119,10 @@ contract PermissionManager is IPermissionManager, Ownable { { uint256[] memory roles = new uint256[](256); uint256 rolesCount = 0; - uint256 accountPermissions = _permissions[account]; + uint256 userPermissions = _users[user].permissions; for (uint256 i = 0; i < 256; i++) { - if ((accountPermissions >> i) & 1 > 0) { + if ((userPermissions >> i) & 1 > 0) { roles[rolesCount] = i; rolesCount++; } @@ -104,26 +132,34 @@ contract PermissionManager is IPermissionManager, Ownable { } ///@inheritdoc IPermissionManager - function isInRole(address account, uint256 role) external view override returns (bool) { - return (_permissions[account] >> role) & 1 > 0; + function isInRole(address user, uint256 role) external view override returns (bool) { + return (_users[user].permissions >> role) & 1 > 0; } ///@inheritdoc IPermissionManager - function isInAllRoles(address account, uint256[] calldata roles) external view override returns (bool) { - - for(uint256 i=0; i> roles[i]) & 1 == 0){ + function isInAllRoles(address user, uint256[] calldata roles) + external + view + override + returns (bool) + { + for (uint256 i = 0; i < roles.length; i++) { + if ((_users[user].permissions >> roles[i]) & 1 == 0) { return false; - } + } } return true; } ///@inheritdoc IPermissionManager - function isInAnyRole(address account, uint256[] calldata roles) external view override returns (bool) { - - for(uint256 i=0; i> roles[i]) & 1 > 0){ + function isInAnyRole(address user, uint256[] calldata roles) + external + view + override + returns (bool) + { + for (uint256 i = 0; i < roles.length; i++) { + if ((_users[user].permissions >> roles[i]) & 1 > 0) { return true; } } @@ -131,7 +167,12 @@ contract PermissionManager is IPermissionManager, Ownable { } ///@inheritdoc IPermissionManager - function isPermissionsAdmin(address account) public view override returns (bool) { - return _permissionsAdmins[account] > 0; + function isPermissionsAdmin(address admin) public view override returns (bool) { + return _permissionsAdmins[admin] > 0; + } + + ///@inheritdoc IPermissionManager + function getUserPermissionAdmin(address user) external view override returns (address) { + return _users[user].permissionAdmin; } } diff --git a/package-lock.json b/package-lock.json index 7f3c76fd..e5198127 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14793,7 +14793,7 @@ } }, "ethereumjs-abi": { - "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#1a27c59c15ab1e95ee8e5c4ed6ad814c49cc439e", + "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#ee3994657fa7a427238e6ba92a84d0b529bbcde0", "from": "git+https://github.com/ethereumjs/ethereumjs-abi.git", "dev": true, "requires": { diff --git a/test-suites/test-aave/permission-manager.spec.ts b/test-suites/test-aave/permission-manager.spec.ts index 36280593..85fb8c0e 100644 --- a/test-suites/test-aave/permission-manager.spec.ts +++ b/test-suites/test-aave/permission-manager.spec.ts @@ -6,7 +6,9 @@ import { PermissionManager } from '../../types'; makeSuite('Permission manager', (testEnv: TestEnv) => { let permissionManager: PermissionManager; - const DEPOSITOR = 0, BORROWER = 1, LIQUIDATOR = 2; + const DEPOSITOR = 0, + BORROWER = 1, + LIQUIDATOR = 2; before('deploying a new Permission manager', async () => { permissionManager = await deployContract('PermissionManager', []); @@ -25,7 +27,9 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Registers a new depositor', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).addPermissions([DEPOSITOR], [users[0].address]); + await permissionManager + .connect(users[0].signer) + .addPermissions([DEPOSITOR], [users[0].address]); const isDepositor = await permissionManager.isInRole(users[0].address, DEPOSITOR); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); @@ -53,7 +57,9 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Registers a new liquidator', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).addPermissions([LIQUIDATOR], [users[0].address]); + await permissionManager + .connect(users[0].signer) + .addPermissions([LIQUIDATOR], [users[0].address]); const isDepositor = await permissionManager.isInRole(users[0].address, DEPOSITOR); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); @@ -64,14 +70,33 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { expect(isLiquidator).to.be.equal(true); }); + it('Checks the permission admin of the user', async () => { + const { users } = testEnv; + + const permissionAdmin = await permissionManager.getUserPermissionAdmin(users[0].address); + + expect(permissionAdmin).to.be.eq(users[0].address); + }); + + it('Adds user 1 as permission admin; checks user 1 cannot change user 0 permission', async () => { + const { users } = testEnv; + + await permissionManager.addPermissionAdmins([users[1].address]); + + await expect( + permissionManager.connect(users[1].signer).addPermissions([LIQUIDATOR], [users[0].address]) + ).to.be.revertedWith('INVALID_PERMISSIONADMIN'); + + await permissionManager.removePermissionAdmins([users[1].address]); + + }); + it('Checks getPermissions', async () => { const { users } = testEnv; - const { - 0: permissions, - } = await permissionManager.getAccountPermissions(users[0].address); + const { 0: permissions } = await permissionManager.getUserPermissions(users[0].address); - const mappedPermissions = permissions.map(item => item.toString()); + const mappedPermissions = permissions.map((item) => item.toString()); expect(mappedPermissions.indexOf(BORROWER.toString())).to.be.gte(0); expect(mappedPermissions.indexOf(DEPOSITOR.toString())).to.be.gte(0); @@ -81,25 +106,33 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks isInAllRoles (positive test)', async () => { const { users } = testEnv; - const result = await permissionManager.isInAllRoles(users[0].address, [DEPOSITOR, BORROWER, LIQUIDATOR]) + const result = await permissionManager.isInAllRoles(users[0].address, [ + DEPOSITOR, + BORROWER, + LIQUIDATOR, + ]); - expect(result).to.be.equal(true, "Invalid result for isInAllRoles"); + expect(result).to.be.equal(true, 'Invalid result for isInAllRoles'); }); - it('Checks isInAnyRole (positive test)', async () => { const { users } = testEnv; - const result = await permissionManager.isInAnyRole(users[0].address, [DEPOSITOR, BORROWER, LIQUIDATOR]) + const result = await permissionManager.isInAnyRole(users[0].address, [ + DEPOSITOR, + BORROWER, + LIQUIDATOR, + ]); - expect(result).to.be.equal(true, "Invalid result for isInAnyRole"); + expect(result).to.be.equal(true, 'Invalid result for isInAnyRole'); }); - it('Removes the depositor', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).removePermissions([DEPOSITOR], [users[0].address]); + await permissionManager + .connect(users[0].signer) + .removePermissions([DEPOSITOR], [users[0].address]); const isDepositor = await permissionManager.isInRole(users[0].address, DEPOSITOR); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); @@ -113,16 +146,21 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks isInAllRoles (negative test)', async () => { const { users } = testEnv; - const result = await permissionManager.isInAllRoles(users[0].address, [DEPOSITOR, BORROWER, LIQUIDATOR]) + const result = await permissionManager.isInAllRoles(users[0].address, [ + DEPOSITOR, + BORROWER, + LIQUIDATOR, + ]); - expect(result).to.be.equal(false, "Invalid result for isInAllRoles"); + expect(result).to.be.equal(false, 'Invalid result for isInAllRoles'); }); - it('Removes the borrower', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).removePermissions([BORROWER], [users[0].address]); + await permissionManager + .connect(users[0].signer) + .removePermissions([BORROWER], [users[0].address]); const isDepositor = await permissionManager.isInRole(users[0].address, DEPOSITOR); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); @@ -136,15 +174,17 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks isInAnyRole (negative test)', async () => { const { users } = testEnv; - const result = await permissionManager.isInAnyRole(users[0].address, [DEPOSITOR, BORROWER]) + const result = await permissionManager.isInAnyRole(users[0].address, [DEPOSITOR, BORROWER]); - expect(result).to.be.equal(false, "Invalid result for isInAnyRole"); + expect(result).to.be.equal(false, 'Invalid result for isInAnyRole'); }); it('Removes the liquidator', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).removePermissions([LIQUIDATOR], [users[0].address]); + await permissionManager + .connect(users[0].signer) + .removePermissions([LIQUIDATOR], [users[0].address]); const isDepositor = await permissionManager.isInRole(users[0].address, DEPOSITOR); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); @@ -158,10 +198,7 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks getPermissions', async () => { const { users } = testEnv; - const { - 1: permissionsCount - } = await permissionManager.getAccountPermissions(users[0].address); - + const { 1: permissionsCount } = await permissionManager.getUserPermissions(users[0].address); expect(permissionsCount).to.be.equal(0); }); @@ -169,8 +206,7 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks that only the permissions manager can set permissions', async () => { const { users } = testEnv; - await expect(permissionManager.connect(users[1].signer).addPermissions([], [])).to.be - .reverted; + await expect(permissionManager.connect(users[1].signer).addPermissions([], [])).to.be.reverted; await expect(permissionManager.connect(users[1].signer).removePermissions([], [])).to.be .reverted; }); @@ -178,17 +214,13 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { it('Checks that only the owner can add permissions admins', async () => { const { users } = testEnv; - await expect(permissionManager.connect(users[1].signer).addPermissionAdmins([])).to.be - .reverted; - await expect(permissionManager.connect(users[1].signer).addPermissionAdmins([])).to.be - .reverted; + await expect(permissionManager.connect(users[1].signer).addPermissionAdmins([])).to.be.reverted; + await expect(permissionManager.connect(users[1].signer).addPermissionAdmins([])).to.be.reverted; }); - it('Add borrower role to user 0. Removes permission admin to user 0, check permission admin is removed and other permissions are not affected', async () => { const { users } = testEnv; - await permissionManager.connect(users[0].signer).addPermissions([BORROWER], [users[0].address]); await permissionManager.removePermissionAdmins([users[0].address]); @@ -196,7 +228,6 @@ makeSuite('Permission manager', (testEnv: TestEnv) => { const isPermissionAdmin = await permissionManager.isPermissionsAdmin(users[0].address); const isBorrower = await permissionManager.isInRole(users[0].address, BORROWER); - expect(isPermissionAdmin).to.be.equal(false); expect(isBorrower).to.be.equal(true); });