First Flight #16: Mafia Takedown

Summary

The two functions Laundrette::addToTheGang and Laundrette::quiteTheGang require that the Laundrette policy contract is the admin of the Kernel to grant and revoke roles for access control. When changing the Kernel admin, even if just temporarily, it's no longer possible to add gang members or exiting the gang via the Laundrette contract.

Vulnerability Details

There are two functions on the Laundrette policy contract to add and remove gang members from the mafia, Laundrette::addToTheGang and Laundrette::quiteTheGang respectively:

These two functions make use of Kernel::grantRole and Kernel::revokeRole which can only be called by the Kernel::admin. This works fine after the system is deployed because the Laundrette contract instance is set as an admin during deployment:

There's another important role in the Kernel system: The executor is allowed to perform calls to Kernel::executeAction (just like the one in the snippet above). This means, the executor is the account that can change the Kernel::admin.

In our case, when the system deployed, the godfather is the executor. If the executor changes the admin to a different account, both functions Laundrette::addToTheGang and Laundrette::quiteTheGang will no longer work, resulting in no account being able to add or remove gang members via the Laundrette policy.

Of course, it's still possible for the new admin account to call Kernel::grantRole and Kernel::revokeRole directly, but at the very least, any normal gang member will no longer be able to remove themselves, and the godfather will not be able to add gang members.

While changing the admin as an executor should not be an event that will happen on a regular basis, it has to happen at least once unless other actions are taken.

The reason for that is, that the godfather does not have the gangmember role when the system is deployed. But it is required by Laundrette::addToTheGang that the caller is a gang member and the godfather (the executor):

So in order for the godfather to add members, he first has to get the gangmember role. But only the Kernel::admin can grant roles, which is the Laundrette contract and there's no function on the Laundrette contract to grant arbritrary roles to accounts.

Impact

Unless other actions are taken, the godfather has to make some other account he controls the admin, so he can Kernel::grantRole himself the gangmember role. Until he then makes the Laundrette the admin of the system again, both Laundrette::addToTheGang and Laundrette::quitTheGang will revert.

The former can only be called by the godfather anyways, but at the very least, other gang members will not be able quit the gang.

Tools Used

• Manual review

• Foundry for testing

Recommended Mitigation

There's a few ways to go about this:

• either we ensure that the godfather is also a gang member when the system is deployed or

• we remove the requirement that the godfather has to be a gang member in the first place

To go with the first option, all we have to do is granting him the role in Deploy.s.sol:

Now there's at least no immediate reason to change the admin anymore, reducing the likelihood that the mentioned functions on Laundrette will always revert.

However, the second option seems to be better. Requiring that the caller has to be a gang member and the godfather to add new gang members seems like a bug. The godfather should be able to add members via Laundrette without having the gangmember role:

We now still have the issue that the functions on Laundrette will not work if the Laundrette contract is not the admin, however we've reduced the incentive to make a change to the admin in the first place.

Proof of Concept

To demonstrate the scenario add the following test to test/Laundrette.t.sol:

Then, drop in this test, which will pass: