Beginner FriendlyDeFiFoundry
100 EXP
View results
Submission Details
Severity: low
Valid

Function to retrieve admin status does not work due to missing privileges

Summary

The Laundrette contract provides a function retrieveAdmin() which is meant to be used by the godfather to gain admin status in the system. This can be necessary when the godfather needs the privileges to grant and revoke roles, as only the Kernel::admin is allowed to do that. However, Laundrette::retrieveAdmin is not able to change the admin because it doesn't have the executor status.

Vulnerability Details

There are types of roles in the mafia takedown contracts:

  • Built-in roles, of which there are Kernel::admin and Kernel::executor

  • Custom roles, which are created via Kernel::grantRole

What's important to note here, is that only Kernel::admin is able to grant and revoke roles. Kernel::executor on the other hand, has the ability to execute actions via Kernel::executeAction, such as Actions.InstallModule, Actions.ActivatePolicy, but also actions related to built-in roles, such as Actions.ChangeAdmin and Actions.ChangeExecutor.

When the system is deployed, the Laundrette contracts becomes the admin while the godfather becomes the executor:

kernel.executeAction(Actions.ChangeAdmin, address(laundrette));
kernel.executeAction(Actions.ChangeExecutor, godFather);

This means, Laundrette can grant and revoke roles, godFather can execute actions.

The Laundrette::retrieveAdmin function, which is meant to be used by the godfather to gain admin status, makes use of Kernel::executeAction to change the admin:

function retrieveAdmin() external {
kernel.executeAction(Actions.ChangeAdmin, kernel.executor());
}

As we've learned however, Laundrette has to be the executor to call Kernel::executeAction. Only the godfather can call Kernel::executeAction.

This function will revert no matter who calls it.

Impact

There's no serious impact here because the only thing that happens is that the function will always revert. The godfather can just call Kernel::executeAction(Actions.ChangeAdmin, Kernel::executor) directly and gains admin status like that.

Tools Used

  • Manual review

  • Foundry for tests

Recommended Mitigation

For this to work, Laundrette would need to become the executor as well. In addition, the account used in Kernel::executeAction needs to be parametrized. The godfather can then use the ChangeExecutor action to make Laundrette the executor, which would make Laundrette::retrieveAdmin work again.

However, this is not recommended because at that point, Laundrette would be the executor and there's no way to give that status to a different account. In addition, this would require additional access control though because that would allow anyone to call this function with any account to become a new admin.

Instead, I'd recommend to just remove Laundrette::retrieveAdmin altogether and simply let the executor call Kernel::executeAction directly to make any changes to Kernel::admin.

- function retrieveAdmin() external {
- kernel.executeAction(Actions.ChangeAdmin, kernel.executor());
- }

Proof of Concept

Below is a test that can be added to Laundrette.t.sol to verify that Laundrette::retrieveAdmin always fails:

+ function test_retrieveAdminBug() public {
+ vm.prank(godFather);
+ vm.expectRevert(abi.encodeWithSignature("Kernel_OnlyExecutor(address)", laundrette));
+ laundrette.retrieveAdmin();
+ }
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`retrieveAdmin` not working

billobaggebilleyan Auditor
about 1 year ago
n0kto Lead Judge
about 1 year ago
n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`retrieveAdmin` not working

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.