Backd contest - hubble's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/access/RoleManager.sol#L43-L46 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/access/RoleManager.sol#L115-L128

Vulnerability details

Impact

The impact of this vulnerability, i.e., losing all governance control is very High. There is a possibility, due to a corner case as described below.

Proof of Concept

Contract : RoleManager.sol Function : renounceGovernance()

   Step 0:
     Let current governance role given to = CURRENT_GOV_ADDRESS
     so, getRoleMemberCount() for "governance" role will return = 1 

   Step 1: Add a new address say ALICE to governance role, by addGovernor(ALICE)
     now, ALICE also has governace role, and getRoleMemberCount() for "governance" role will return = 2 
   
   Step 2: Assume that ALICE renounces governance role, by renounceGovernance()
     now, ALICE does not have governance role, but getRoleMemberCount() for "governance" role will return = 2, due to a BUG

   Step 3: In some distant future, if there is a compromise of CURRENT_GOV_ADDRESS keys or due to some other reason, 
     its decided to revoke governance role for CURRENT_GOV_ADDRESS via renounceGovernance(), and the command succeeds
     It can be assumed that since getRoleMemberCount() for "governance" role returns = 2, at least there is one other active governor address. 
     But now, CURRENT_GOV_ADDRESS does not have governance role, and the total governance control on the protocol is lost my mistake.

Recommended Mitigation Steps

getRoleMemberCount() currently returns _roleMembers[role].length(); It should return the count only for _roles[role].members[account] = true;

Its recommended to add a new function to know who are the active members for any role, like getRoleMembers(bytes32 role) returning address account []

Summary of Findings for Low / Non-Critical issues

• L-01 : Inconsistencies in constructor parameters of the contracts

• L-02 : No reset equivalent for function prepareKeeperRequiredStakedBKD

• L-03 : Wrong use of amount in the emit Deposit event in VaultReserve.sol

• L-04 : Upgradability of the core contracts not considered

• NC-01 : modifier onlyPool not used anywhere in Vault contract

• NC-02 : Wrong and misleading dev comment in AddressProvider.sol

Details L-01

Title : Inconsistencies in constructor parameters of the contracts

The constructors for Controller, CvxCrvRewardsLocker, etc., use 'IAddressProvider _addressProvider' as parameter, and other values are obtained from the addressProvider functions.

On the contrary, the constructors for TopUpAction, Vault, LiquidityPool, GasBank, etc., use 'IController _controller' as parameter, and other values like addressProvider are derived from the controller functions.

There is a possibility of maliciously deploying multiple controllers, (though each can use same or different addressProvider), and have multiple sources of truth.

Recommended Mitigation Steps

Consistently use addressProvider as the contract parameter and derive other values from it, since the AddressProvider contract is a single source of truth. This will probibit any misuse or mistakes during deployment of contracts in future.

Details L-02

Title : No reset equivalent for function prepareKeeperRequiredStakedBKD

Impact

The protocol will not be able to reset the values that was prepared for KEEPER_REQUIRED_STAKED_BKD. In case reset is required, it has to be executed and prepared again for correcting any mistake.

Proof of Concept

Contract : Controller.sol Function : prepareKeeperRequiredStakedBKDO() and executeKeeperRequiredStakedBKD()

Recommended Mitigation Steps

Define a new function resetKeeperRequiredStakedBKD, to be consistent with the prepare, reset and execute pattern.

Details L-03

Title : Wrong use of amount in the emit Deposit event in VaultReserve.sol

In the emit event of Deposit() function, instead of using 'amount' , the correct value to use is 'received'

Impact

Possibility of emitting a wrong value in case the value of 'received' is more than 'amount'.

   uint256 received = newBalance - balance;
   require(received >= amount, Error.INVALID_AMOUNT);
   _balances[msg.sender][token] += received;

Proof of Concept

Contract : VaultReserve.sol Function : deposit

line 61 emit Deposit(msg.sender, token, amount);

Recommended Mitigation Steps

Change as below

line 61 emit Deposit(msg.sender, token, received);

Details L-04

Title : Upgradability of the core contracts not considered

Since the protocol is still in development, the features and functions may be needed to be modified/added in say the AddressProvider, Controller, etc., In that eventuality, it may be difficult to upgrade these contracts.

Recommended Mitigation Steps

Use upgradable proxy pattern of deployment for AddressProvider, Controller etc.,

Details NC-01

Title : modifier onlyPool not used anywhere in Vault contract

Proof of Concept

Contract : Vault.sol
modifier onlyPool() { require(msg.sender == pool, Error.UNAUTHORIZED_ACCESS); _; }

Recommended Mitigation Steps

Since its a dead code, delete the modifier onlyPool()

Details NC-02

Title : Wrong and misleading dev comment in AddressProvider.sol

Proof of Concept

Contract : AddressProvider.sol Function : addStakerVault()

line 271 /** line 272 * @notice Add a new staker vault and add it's lpGauge if set in vault.

There is no lpGause set in this contract, but its set in Controller.sol for the same function.

Recommended Mitigation Steps

Move the @notice comment to against the function addStakerVault in Controller.sol contract where its relevant.