search

code-423n4 / 2022-04-backd-findings

6 stars 4 forks source link

Its possible to lose total governance control by mistake #83

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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 []

chase-manning commented 2 years ago

Duplicate of #164