[Epic] Access Control and Roles

[iafhurtado]

The goal of this ticket is to have access controls and roles implemented in a consistent way across the whole system. This is a ticket to track progress.

1. Chief

• [x] Owner is a multisig that create, grants and revokes roles see comment
• [x] store all roles in a mapping: Treasury, Rebalancer, Liquidator, Harvester, Pauser? and AdminTimelock?, see comment
• [ ] createRole(role) onlyOwner
• [x] hasRole(role, address) returns (bool) - check if the address has the specified role
• [x] grantRole(role, address) onlyOwner - give a role to the address
• [x] revokeRole(role, address) onlyOwner - revoke role of the address

2. BorrowingVault and YieldVault

• [x] implement pause() and unpause() with allowed callers or an address having the role Pauser
• [x] setOracle(), setMaxLtv() and setLiqRatio() can be called only by an address with AdminTimelock role
• [x] liquidate() can be called only by an address with Liquidator role
• [x] setProviders() can be called only by an address with AdminTimelock role
• [x] setActiveProvider() can be called only by an address with Rebalancer role (attention this can break existing tests)
• [x] harvest() - will be created after we deal with the topic "HarvestManager"

3. FujiOracle

• [x] setPriceFeed() - can be called only by an address with AdminTimelock role

4. AddrMapper - check the comments below

• [x] create mapping providerName => ( address1 => (address2 => IBT address)) - providerName has the format Name_Version or Name_Version_Asset (for ex. Aave_V2 but not Aave_v2, BeefyVelodrome_V6_sETHETH)
• [x] store all providerName in a array and add a getter function
• [x] setter function can be called by an address with AdminTimelock role

[brozorec]

Contract	Permissioned functions	Allowed caller	Notes
BorrowingVault
	pause()	Chief	From pauseAllVaults() in case of emergency.
	pause()	MultiSig
	unpause()	Chief	From unpauseAllVaults().
	unpause()	MultiSig
	setOracle()	MultiSig -timelock
	setMaxLtv()	MultiSig-timelock
	setLiqRatio()	MultiSig-timelock
	liquidate()	LiquidationManager	A new contract that manages a list of allowed liquidators. liquidate() needs to call the Chief to check if msg.sender has a "Liquidator" role.
	rebalance()	RebalanceManager	A new contract that manages a list of allowed rebalancers and handles flashloan logic. rebalance() needs to call the Chief to check if msg.sender has a "Rebalancer" role.
	harvest()	HarvestManager	A new contract that manages a list of allowed harvesters and handles specificities for each protocol. harvest() needs to call the Chief to check if msg.sender has a "Harvester" role.
	setProviders()	MultiSig-timelock
	setActiveProvider()	RebalanceManager	A new contract that manages a list of allowed rebalancers and handles flashloan logic. setActiveProvider() needs to call the Chief to check if msg.sender has a "Rebalancer" role.
YieldVault
	pause()	Chief	from pauseAllVaults() in case of emergency
	pause()	MultiSig
	unpause()	Chief	from unpauseAllVaults()
	unpause()	MultiSig
	rebalance()	RebalanceManager	a contract that manages a list of allowed rebalancers and handles flashloan logic
	harvest()	HarvestManager	a contract that manages a list of allowed harvesters and handles specificities for each protocol
VaultDeployer
	_registerVault()	Chief	Internal function called by every vault factory.
FujiOracle
	setPriceFeed()	MultiSig-timelock
Chief
	deployVault()	EOA	We start with only allowed EOAs that can depoy a new vault and eventually, open it for any EOA.
	pauseAllVaults()	MultiSig
	unpauseAllVaults()	MultiSig
	grantRole()	MultiSig	ref: https://docs.openzeppelin.com/contracts/4.x/api/access
	revokeRole()	MultiSig	ref: https://docs.openzeppelin.com/contracts/4.x/api/access
	addToAllowed()	MultiSig	Controls allowed factories for vaults.
	removeFromAllowed()	MultiSig	Controls allowed factories for vaults.
AddrMapper *
	setMapping()	MultiSig
	setNestedMapping()	MultiSig
Flasher
	N/A	N/A
Swapper
	N/A	N/A

---

• "AddrMapper" is used in some provider contracts as a helper contract to map ERC20s to their IBT equivalents for each provider. We need to deploy a new "AddrMapper" for every new provider contract that requires such mapping. We need to consider having only one "AddrMapper" for this purpose as this will greatly simplify management.

@DaigaroCota plz check

[0xdcota]

@brozorec , first awesome job on the detailing of the functions and the intended role. Some comments:

1. For pause() I am thinking we also add Externally Owned Accounts (yours or mine) for quick reaction. Or an alternative is that we create a "Security Multisg" including 1 signer from the auditors of the code. To also have them have some response responsibility. We can discuss the last point further.
2. Excellent and completely agree in the need of pauseAllVaults() and viceverse. Only question I will have, is if we introduce and array to iterate over? Or we keep track of all vaults off-chain?
3. I added Timelock to some of the functions that in the future I suggest should have this time component to allow people to react if they would like to react.
4. For the deployVault() function in the Chief, in response to the comments, I would rather advocate to this be done by a future governance process instead of just "any EOA". The reason is that users could bloat our system with not relevant vaults, better to perhaps have a vetting process, because there is some risk associated with setting up the providers. Is not like Curve in where they are the lowest block and all risk lies within their code.
5. For the AddrMapper you think perhaps we include a double nested mapping? For example: (string) providerName => ( addressKey1 => (addressKey2 => IBT equivalent address)) I believe this way the mapper can be the same address and just keep adding maps.

[brozorec]

@iafhurtado @DaigaroCota fyi this ticket's description got updated with the needed details and an action plan.

[brozorec]

For pause() I am thinking we also add Externally Owned Accounts (yours or mine) for quick reaction.

1. I suggest creating a Pauser role that can be attributed to EOAs.

Excellent and completely agree in the need of pauseAllVaults() and viceverse. Only question I will have, is if we introduce and array to iterate over? Or we keep track of all vaults off-chain?

2. Yes, we keep them in an array in the contract.

3. I agree

4. I agree

5. Added specs in the ticket description.

@DaigaroCota

[iafhurtado]

Time estimation around 2 days

[0xdcota]

@brozorec After getting into the writing of the actual implementations, I have the following comments: 1.- Openzeppelin-AccessControl.sol can be used entirely to replace the Ownable.sol. DEFAULT_ADMIN_ROLE can be used instead of onlyOwner. This will avoid inheriting Ownable, and hence having two "access-control" implementations in Chief. 2.- I followed the naming convention for roles that was established by OpenZeppelin. NAME_ROLE. 3.- I decided to split the roles into two groups. The "core roles" are the ones that will be hardcoded in the contract system. Any additional role can be created by the intended createRole() functions. Additionally, I assumed this createRole() is intended to add roles in the future, like we did with the game roles in V1-nft-game. The reason to split it, is because it simplifies the modifier call, and costs less gas to know the roles before hand. That way, you can do, chief.hasRole(who, role) and is simple at runtime. Otherwise, If not hardcoded, you need to check; role exists? or some type of getRole(this).

[brozorec]

1. and 2. very well
2. I started looking into it, very good ideas overall :+1: I have some feedback on the organization of the new code, I'll write it down directly in #94 @DaigaroCota