feat: Add generic RBAC for GHO token (aave#332)

cedephrase · web-flow · commit a83493836103 · 2023-05-16T14:13:07.000+02:00
* feat: initial generic RBAC for GHO token

* fix: use new roles exclusively, update tests for roles

* fix: change certora patch

* fix: bug in previous certora fix

* fix: add admin param to GhoToken constructor

* fix: modify Certora GhoToken harness for new admin param

* fix: add role grant verification
diff --git a/certora/applyHarness.patch b/certora/applyHarness.patch
@@ -7,12 +7,12 @@ diff -ruN ../src/.gitignore .gitignore
 diff -ruN ../src/contracts/gho/GhoToken.sol contracts/gho/GhoToken.sol
 --- ../src/contracts/gho/GhoToken.sol	2023-02-26 10:23:14.000000000 +0200
 +++ contracts/gho/GhoToken.sol	2023-02-26 13:26:13.000000000 +0200
-@@ -71,11 +71,16 @@
+@@ -75,11 +75,16 @@
      uint128 bucketCapacity
-   ) external onlyOwner {
+   ) external onlyRole(FACILITATOR_MANAGER) {
      Facilitator storage facilitator = _facilitators[facilitatorAddress];
 +    require(
-+      !facilitator.isLabelNonempty,    //TODO: remove workaroun when CERT-977 is resolved
++      !facilitator.isLabelNonempty, //TODO: remove workaroun when CERT-977 is resolved
 +      'FACILITATOR_ALREADY_EXISTS'
 +    );
      require(bytes(facilitator.label).length == 0, 'FACILITATOR_ALREADY_EXISTS');
@@ -24,29 +24,29 @@ diff -ruN ../src/contracts/gho/GhoToken.sol contracts/gho/GhoToken.sol
  
      _facilitatorsList.add(facilitatorAddress);
  
-@@ -89,6 +94,10 @@
+@@ -93,6 +98,10 @@
    /// @inheritdoc IGhoToken
-   function removeFacilitator(address facilitatorAddress) external onlyOwner {
+   function removeFacilitator(address facilitatorAddress) external onlyRole(FACILITATOR_MANAGER) {
      require(
-+      _facilitators[facilitatorAddress].isLabelNonempty,    //TODO: remove workaroun when CERT-977 is resolved
++      _facilitators[facilitatorAddress].isLabelNonempty, //TODO: remove workaroun when CERT-977 is resolved
 +      'FACILITATOR_DOES_NOT_EXIST'
 +    );
 +    require(
        bytes(_facilitators[facilitatorAddress].label).length > 0,
        'FACILITATOR_DOES_NOT_EXIST'
      );
-@@ -108,6 +117,10 @@
+@@ -112,6 +121,10 @@
      address facilitator,
      uint128 newCapacity
-   ) external onlyOwner {
+   ) external onlyRole(BUCKET_MANAGER) {
 +    require(
-+      _facilitators[facilitator].isLabelNonempty,    //TODO: remove workaroun when CERT-977 is resolved
++      _facilitators[facilitator].isLabelNonempty, //TODO: remove workaroun when CERT-977 is resolved
 +      'FACILITATOR_DOES_NOT_EXIST'
 +    );
      require(bytes(_facilitators[facilitator].label).length > 0, 'FACILITATOR_DOES_NOT_EXIST');
  
      uint256 oldCapacity = _facilitators[facilitator].bucketCapacity;
-@@ -122,12 +135,12 @@
+@@ -126,12 +139,12 @@
    }
  
    /// @inheritdoc IGhoToken
diff --git a/certora/harness/GhoTokenHarness.sol b/certora/harness/GhoTokenHarness.sol
@@ -7,6 +7,8 @@ import {GhoToken} from '../munged/contracts/gho/GhoToken.sol';
 contract GhoTokenHarness is GhoToken {
   using EnumerableSet for EnumerableSet.AddressSet;
 
+  constructor() GhoToken(msg.sender) {}
+
   /**
    * @notice Returns the backet capacity
    * @param facilitator The address of the facilitator
diff --git a/deploy/00_deploy_gho_token.ts b/deploy/00_deploy_gho_token.ts
@@ -16,7 +16,7 @@ const func: DeployFunction = async function ({
 
   const ghoResult = await deploy('GhoToken', {
     from: deployer,
-    args: [],
+    args: [deployer],
     log: true,
   });
   console.log(`GHO Address:                   ${ghoResult.address}`);
diff --git a/src/contracts/gho/GhoToken.sol b/src/contracts/gho/GhoToken.sol
@@ -2,25 +2,29 @@
 pragma solidity ^0.8.0;
 
 import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol';
-import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
+import {AccessControl} from '@openzeppelin/contracts/access/AccessControl.sol';
 import {ERC20} from './ERC20.sol';
 import {IGhoToken} from './interfaces/IGhoToken.sol';
 
 /**
  * @title GHO Token
  * @author Aave
  */
-contract GhoToken is ERC20, Ownable, IGhoToken {
+contract GhoToken is ERC20, AccessControl, IGhoToken {
   using EnumerableSet for EnumerableSet.AddressSet;
 
   mapping(address => Facilitator) internal _facilitators;
   EnumerableSet.AddressSet internal _facilitatorsList;
 
+  bytes32 public constant FACILITATOR_MANAGER = keccak256('FACILITATOR_MANAGER');
+  bytes32 public constant BUCKET_MANAGER = keccak256('BUCKET_MANAGER');
+
   /**
    * @dev Constructor
+   * @param admin This is the initial holder of the default admin role
    */
-  constructor() ERC20('Gho Token', 'GHO', 18) {
-    // Intentionally left blank
+  constructor(address admin) ERC20('Gho Token', 'GHO', 18) {
+    _setupRole(DEFAULT_ADMIN_ROLE, admin);
   }
 
   /**
@@ -69,7 +73,7 @@ contract GhoToken is ERC20, Ownable, IGhoToken {
     address facilitatorAddress,
     string calldata facilitatorLabel,
     uint128 bucketCapacity
-  ) external onlyOwner {
+  ) external onlyRole(FACILITATOR_MANAGER) {
     Facilitator storage facilitator = _facilitators[facilitatorAddress];
     require(bytes(facilitator.label).length == 0, 'FACILITATOR_ALREADY_EXISTS');
     require(bytes(facilitatorLabel).length > 0, 'INVALID_LABEL');
@@ -87,7 +91,7 @@ contract GhoToken is ERC20, Ownable, IGhoToken {
   }
 
   /// @inheritdoc IGhoToken
-  function removeFacilitator(address facilitatorAddress) external onlyOwner {
+  function removeFacilitator(address facilitatorAddress) external onlyRole(FACILITATOR_MANAGER) {
     require(
       bytes(_facilitators[facilitatorAddress].label).length > 0,
       'FACILITATOR_DOES_NOT_EXIST'
@@ -107,7 +111,7 @@ contract GhoToken is ERC20, Ownable, IGhoToken {
   function setFacilitatorBucketCapacity(
     address facilitator,
     uint128 newCapacity
-  ) external onlyOwner {
+  ) external onlyRole(BUCKET_MANAGER) {
     require(bytes(_facilitators[facilitator].label).length > 0, 'FACILITATOR_DOES_NOT_EXIST');
 
     uint256 oldCapacity = _facilitators[facilitator].bucketCapacity;
diff --git a/src/test/TestGhoBase.t.sol b/src/test/TestGhoBase.t.sol
@@ -104,7 +104,9 @@ contract TestGhoBase is Test, Constants, Events {
     CONFIGURATOR = new MockedConfigurator(IPool(POOL));
     GHO_ORACLE = new GhoOracle();
     GHO_MANAGER = new GhoManager();
-    GHO_TOKEN = new GhoToken();
+    GHO_TOKEN = new GhoToken(address(this));
+    GHO_TOKEN.grantRole(FACILITATOR_MANAGER, address(this));
+    GHO_TOKEN.grantRole(BUCKET_MANAGER, address(this));
     AAVE_TOKEN = new TestnetERC20('AAVE', 'AAVE', 18, FAUCET);
     StakedAaveV3 stkAave = new StakedAaveV3(
       IERC20(address(AAVE_TOKEN)),
diff --git a/src/test/TestGhoToken.t.sol b/src/test/TestGhoToken.t.sol
@@ -2,10 +2,14 @@
 pragma solidity ^0.8.0;
 
 import './TestGhoBase.t.sol';
+import '@openzeppelin/contracts/utils/Strings.sol';
 
 contract TestGhoToken is TestGhoBase {
   function testConstructor() public {
-    GhoToken ghoToken = new GhoToken();
+    GhoToken ghoToken = new GhoToken(address(this));
+    vm.expectEmit(true, true, true, true, address(GHO_TOKEN));
+    emit RoleGranted(GHO_TOKEN.DEFAULT_ADMIN_ROLE(), msg.sender, address(this));
+    GHO_TOKEN.grantRole(GHO_TOKEN.DEFAULT_ADMIN_ROLE(), msg.sender);
     assertEq(ghoToken.name(), 'Gho Token', 'Wrong default ERC20 name');
     assertEq(ghoToken.symbol(), 'GHO', 'Wrong default ERC20 symbol');
     assertEq(ghoToken.decimals(), 18, 'Wrong default ERC20 decimals');
@@ -61,6 +65,16 @@ contract TestGhoToken is TestGhoBase {
     GHO_TOKEN.addFacilitator(ALICE, 'Alice', DEFAULT_CAPACITY);
   }
 
+  function testAddFacilitatorWithRole() public {
+    vm.expectEmit(true, true, true, true, address(GHO_TOKEN));
+    emit RoleGranted(GHO_TOKEN.FACILITATOR_MANAGER(), ALICE, address(this));
+    GHO_TOKEN.grantRole(GHO_TOKEN.FACILITATOR_MANAGER(), ALICE);
+    vm.prank(ALICE);
+    vm.expectEmit(true, true, false, true, address(GHO_TOKEN));
+    emit FacilitatorAdded(ALICE, keccak256(abi.encodePacked('Alice')), DEFAULT_CAPACITY);
+    GHO_TOKEN.addFacilitator(ALICE, 'Alice', DEFAULT_CAPACITY);
+  }
+
   function testRevertAddExistingFacilitator() public {
     vm.expectRevert('FACILITATOR_ALREADY_EXISTS');
     GHO_TOKEN.addFacilitator(address(GHO_ATOKEN), 'Aave V3 Pool', DEFAULT_CAPACITY);
@@ -71,6 +85,18 @@ contract TestGhoToken is TestGhoBase {
     GHO_TOKEN.addFacilitator(ALICE, '', DEFAULT_CAPACITY);
   }
 
+  function testRevertAddFacilitatorNoRole() public {
+    bytes memory revertMsg = abi.encodePacked(
+      'AccessControl: account ',
+      Strings.toHexString(ALICE),
+      ' is missing role ',
+      Strings.toHexString(uint256(FACILITATOR_MANAGER), 32)
+    );
+    vm.prank(ALICE);
+    vm.expectRevert(revertMsg);
+    GHO_TOKEN.addFacilitator(ALICE, 'Alice', DEFAULT_CAPACITY);
+  }
+
   function testRevertSetBucketCapacityNonFacilitator() public {
     vm.expectRevert('FACILITATOR_DOES_NOT_EXIST');
     GHO_TOKEN.setFacilitatorBucketCapacity(ALICE, DEFAULT_CAPACITY);
@@ -82,6 +108,28 @@ contract TestGhoToken is TestGhoBase {
     GHO_TOKEN.setFacilitatorBucketCapacity(address(GHO_ATOKEN), 0);
   }
 
+  function testSetNewBucketCapacityAsManager() public {
+    vm.expectEmit(true, true, true, true, address(GHO_TOKEN));
+    emit RoleGranted(GHO_TOKEN.BUCKET_MANAGER(), ALICE, address(this));
+    GHO_TOKEN.grantRole(GHO_TOKEN.BUCKET_MANAGER(), ALICE);
+    vm.prank(ALICE);
+    vm.expectEmit(true, false, false, true, address(GHO_TOKEN));
+    emit FacilitatorBucketCapacityUpdated(address(GHO_ATOKEN), DEFAULT_CAPACITY, 0);
+    GHO_TOKEN.setFacilitatorBucketCapacity(address(GHO_ATOKEN), 0);
+  }
+
+  function testRevertSetNewBucketCapacityNoRole() public {
+    bytes memory revertMsg = abi.encodePacked(
+      'AccessControl: account ',
+      Strings.toHexString(ALICE),
+      ' is missing role ',
+      Strings.toHexString(uint256(BUCKET_MANAGER), 32)
+    );
+    vm.prank(ALICE);
+    vm.expectRevert(revertMsg);
+    GHO_TOKEN.setFacilitatorBucketCapacity(address(GHO_ATOKEN), 0);
+  }
+
   function testRevertRemoveNonFacilitator() public {
     vm.expectRevert('FACILITATOR_DOES_NOT_EXIST');
     GHO_TOKEN.removeFacilitator(ALICE);
@@ -99,6 +147,28 @@ contract TestGhoToken is TestGhoBase {
     GHO_TOKEN.removeFacilitator(address(GHO_ATOKEN));
   }
 
+  function testRemoveFacilitatorWithRole() public {
+    vm.expectEmit(true, true, true, true, address(GHO_TOKEN));
+    emit RoleGranted(GHO_TOKEN.FACILITATOR_MANAGER(), ALICE, address(this));
+    GHO_TOKEN.grantRole(GHO_TOKEN.FACILITATOR_MANAGER(), ALICE);
+    vm.prank(ALICE);
+    vm.expectEmit(true, false, false, true, address(GHO_TOKEN));
+    emit FacilitatorRemoved(address(GHO_ATOKEN));
+    GHO_TOKEN.removeFacilitator(address(GHO_ATOKEN));
+  }
+
+  function testRevertRemoveFacilitatorNoRole() public {
+    bytes memory revertMsg = abi.encodePacked(
+      'AccessControl: account ',
+      Strings.toHexString(ALICE),
+      ' is missing role ',
+      Strings.toHexString(uint256(FACILITATOR_MANAGER), 32)
+    );
+    vm.prank(ALICE);
+    vm.expectRevert(revertMsg);
+    GHO_TOKEN.removeFacilitator(address(GHO_ATOKEN));
+  }
+
   function testRevertMintBadFacilitator() public {
     vm.prank(ALICE);
     vm.expectRevert('INVALID_FACILITATOR');
diff --git a/src/test/helpers/Constants.sol b/src/test/helpers/Constants.sol
@@ -6,6 +6,10 @@ contract Constants {
   address constant SHORT_EXECUTOR = 0xEE56e2B3D491590B5b31738cC34d5232F378a8D5;
   address constant STKAAVE_PROXY_ADMIN = 0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF;
 
+  // admin roles for GhoToken
+  bytes32 public constant FACILITATOR_MANAGER = keccak256('FACILITATOR_MANAGER');
+  bytes32 public constant BUCKET_MANAGER = keccak256('BUCKET_MANAGER');
+
   // defaults used in test environment
   uint256 constant DEFAULT_FLASH_FEE = 0.0009e4; // 0.09%
   uint128 constant DEFAULT_CAPACITY = 100_000_000e18;
diff --git a/src/test/helpers/Events.sol b/src/test/helpers/Events.sol
@@ -72,4 +72,7 @@ interface Events {
     address indexed asset,
     uint256 amount
   );
+
+  event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
+  event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
 }
diff --git a/tasks/roles/00_gho-transfer-ownership.ts b/tasks/roles/00_gho-transfer-ownership.ts
@@ -1,12 +1,16 @@
+import { expect } from 'chai';
 import { GhoToken } from './../../../types/src/contracts/gho/GhoToken';
 import { task } from 'hardhat/config';
 
 task('gho-transfer-ownership', 'Transfer Ownership of Gho')
   .addParam('newOwner')
   .setAction(async ({ newOwner }, hre) => {
+    const DEFAULT_ADMIN_ROLE = hre.ethers.utils.hexZeroPad('0x00', 32);
     const gho = (await hre.ethers.getContract('GhoToken')) as GhoToken;
-    const transferOwnershipTx = await gho.transferOwnership(newOwner);
-    await transferOwnershipTx.wait();
+    const grantAdminRoleTx = await gho.grantRole(DEFAULT_ADMIN_ROLE, newOwner);
+    await expect(grantAdminRoleTx).to.emit(gho, 'RoleGranted');
+
+    const removeAdminRoleTx = await gho.renounceRole(DEFAULT_ADMIN_ROLE, users[0].address);
 
     console.log(`GHO ownership transferred to:  ${newOwner}`);
   });
diff --git a/tasks/testnet-setup/00_initialize-gho-reserve.ts b/tasks/testnet-setup/00_initialize-gho-reserve.ts
@@ -24,6 +24,8 @@ task('initialize-gho-reserve', 'Initialize Gho Reserve').setAction(async (_, hre
   const ghoVariableDebtTokenImplementation = await ethers.getContract('GhoVariableDebtToken');
   const ghoInterestRateStrategy = await ethers.getContract('GhoInterestRateStrategy');
   const ghoToken = await ethers.getContract('GhoToken');
+  ghoToken.grantRole(ghoToken.FACILITATOR_MANAGER(), _deployer.address);
+  ghoToken.grantRole(ghoToken.BUCKET_MANAGER(), _deployer.address);
   const poolConfigurator = await getPoolConfiguratorProxy();
   const treasuryAddress = (await hre.deployments.get(TREASURY_PROXY_ID)).address;
   const incentivesControllerAddress = (await hre.deployments.get(INCENTIVES_PROXY_ID)).address;
diff --git a/test/flashmint.test.ts b/test/flashmint.test.ts
@@ -8,6 +8,7 @@ import { ghoEntityConfig } from '../helpers/config';
 import { mintErc20 } from './helpers/user-setup';
 import './helpers/math/wadraymath';
 import { evmRevert, evmSnapshot } from '../helpers/misc-utils';
+import { keccak256, toUtf8Bytes } from 'ethers/lib/utils';
 
 makeSuite('Gho FlashMinter', (testEnv: TestEnv) => {
   let ethers;
@@ -162,16 +163,21 @@ makeSuite('Gho FlashMinter', (testEnv: TestEnv) => {
   it('Flashmint and change capacity mid-execution as approved FlashBorrower', async function () {
     const snapId = await evmSnapshot();
 
-    const { flashMinter, gho, ghoOwner, aclAdmin, aclManager } = testEnv;
+    const { flashMinter, gho, ghoOwner, aclAdmin, aclManager, users } = testEnv;
 
     expect(await aclManager.isFlashBorrower(flashBorrower.address)).to.be.false;
 
     await aclManager.connect(aclAdmin.signer).addFlashBorrower(flashBorrower.address);
 
     expect(await aclManager.isFlashBorrower(flashBorrower.address)).to.be.true;
 
-    await expect(gho.connect(ghoOwner.signer).transferOwnership(flashBorrower.address)).to.not.be
-      .reverted;
+    const BUCKET_MANAGER_ROLE = ethers.utils.hexZeroPad(
+      keccak256(toUtf8Bytes('BUCKET_MANAGER')),
+      32
+    );
+
+    await expect(gho.connect(ghoOwner.signer).grantRole(BUCKET_MANAGER_ROLE, flashBorrower.address))
+      .to.not.be.reverted;
 
     expect((await gho.getFacilitatorBucket(flashMinter.address))[0]).to.not.eq(0);
 
diff --git a/test/gho-token-permit.test.ts b/test/gho-token-permit.test.ts
@@ -6,6 +6,7 @@ import { HardhatEthersHelpers } from '@nomiclabs/hardhat-ethers/types';
 import { BigNumber } from 'ethers';
 import { HARDHAT_CHAINID, MAX_UINT_AMOUNT, ZERO_ADDRESS } from './../helpers/constants';
 import { buildPermitParams, getSignatureFromTypedData } from './helpers/helpers';
+import { keccak256, toUtf8Bytes } from 'ethers/lib/utils';
 
 describe('GhoToken Unit Test', () => {
   let ethers: typeof import('ethers/lib/ethers') & HardhatEthersHelpers;
@@ -53,7 +54,26 @@ describe('GhoToken Unit Test', () => {
   });
 
   it('Deploys GHO and adds the first facilitator', async function () {
-    ghoToken = await ghoTokenFactory.deploy();
+    ghoToken = await ghoTokenFactory.deploy(users[0].address);
+
+    const FACILITATOR_MANAGER_ROLE = ethers.utils.hexZeroPad(
+      keccak256(toUtf8Bytes('FACILITATOR_MANAGER')),
+      32
+    );
+    const BUCKET_MANAGER_ROLE = ethers.utils.hexZeroPad(
+      keccak256(toUtf8Bytes('BUCKET_MANAGER')),
+      32
+    );
+
+    const grantFacilitatorRoleTx = await ghoToken
+      .connect(users[0].signer)
+      .grantRole(FACILITATOR_MANAGER_ROLE, users[0].address);
+    const grantBucketRoleTx = await ghoToken
+      .connect(users[0].signer)
+      .grantRole(BUCKET_MANAGER_ROLE, users[0].address);
+
+    await expect(grantFacilitatorRoleTx).to.emit(ghoToken, 'RoleGranted');
+    await expect(grantBucketRoleTx).to.emit(ghoToken, 'RoleGranted');
 
     const labelHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(facilitator1Label));
 
diff --git a/test/gho-token-unit.test.ts b/test/gho-token-unit.test.ts

Original file line number	Diff line number	Diff line change
`@@ -72,4 +72,7 @@ interface Events {`
`72`	`72`	`address indexed asset,`
`73`	`73`	`uint256 amount`
`74`	`74`	`);`
	`75`	`+`
	`76`	`+ event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);`
	`77`	`+ event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);`
`75`	`78`	`}`