RolesAuthority: Incorrectly removing the function signature from the target.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Dependencies/RolesAuthority.sol#L121

Vulnerability details

Impact

When we restrict one role from invoking a specific function at the target address, we incorrectly label the function as non-callable, even though there are still some roles that can invoke it. This can also result in the removal of the target from the targets array. One function impacted by this issue is the getEnabledFunctionsInTarget function in the Governor contract

Proof of Concept

In the setRoleCapability function, we eliminate the functionSig from enabledFunctionSigsByTarget when the enabled parameter is set to false.

function setRoleCapability(
    uint8 role,
    address target,
    bytes4 functionSig,
    bool enabled
) public virtual requiresAuth {
    if (enabled) {
        ...
    } else {
        getRolesWithCapability[target][functionSig] &= ~bytes32(1 << role);
        enabledFunctionSigsByTarget[target].remove(bytes32(functionSig));

        if (enabledFunctionSigsByTarget[target].length() == 0) {
            targets.remove(target);
        }
    }
}

If there were already at least 2 roles capable of invoking this function, and we attempt to delete one role, there are still other roles available to invoke this function. However, we incorrectly label this function as not callable in the target address.

Tools Used

Recommended Mitigation Steps

Please modify the setRoleCapability function as follows:

    - enabledFunctionSigsByTarget[target].remove(bytes32(functionSig));
    + if (getRolesWithCapability[target][functionSig] == bytes32(0)) {
    +       enabledFunctionSigsByTarget[target].remove(bytes32(functionSig));
    + }

Assessed type

Error

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[jhsagd76]

It's valid, But it only affects a view function getEnabledFunctionsInTarget that hasn't been invoked in the contract, which is not enough to qualify as a med, but it is certainly a valid QA.

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

jhsagd76 marked the issue as grade-a

[c4-judge]

jhsagd76 marked the issue as selected for report

[c4-judge]

jhsagd76 marked the issue as not selected for report