code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L61 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L107

Vulnerability details

Impact

Some roles doesn't have any admin set: TECH_OPERATIONS_ROLE and TREASURY_OPERATIONS_ROLE (1).

Function (2) can change admin of existing roles, as there is no check whether the role exist already. Direct use case would be to change the admin role of DEFAULT_ADMIN_ROLE to something else than CONTRACT_GOVERNANCE_ROLE.

Proof of Concept

hardhat test

Tools Used

(1) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L61 (2) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L107

Recommended Mitigation Steps

First set in the initialize an admin. Second for the override of the admin, it should be check the current admin of the role. If there is already one then revert.

GalloDaSballo commented 2 years ago

I believe the finding to be invalid: 1) Some roles don't have an admin -> I believe the default admin can add any other role

2) The governance (the super admin if you will) can change the admin of any role, that's the purpose of the role contract

I am not fully sure what the warden is trying to imply, the governance role is supposed to change roles