First Flight #16: Mafia Takedown

Summary

There's a bug in Laundrette::configureDependencies() which causes the MoneyShelf module not to be registered correctly as dependency of the Laundrette policy contract. This will prevent the godfather from upgrading the registered MoneyShelf module to the MoneyVault module in case of an emergency.

Vulnerability Details

As part of using the Default framework, "Policies" have to implement a configureDependencies() function that returns the keycodes of all the modules that the policy contract depends on. This is needed so the framework can keep track of which policies need to be reconfigured when modules are upgraded, or pruned when policies are deactivated.

The Laundrette contract is such a policy and implements a dedicated configureDependencies() function:

The keycodes used to identify modules in the system are just bytes5 values. So what happens here is that two references to MoneyShelf and WeaponShelf modules are created, and a list of corresponding dependencies is returned (MONEY and WEAPN keycodes).

However, looking closely, we can see that the entry for the MoneyShelf keycode is overridden by the WeaponShelf keycode. This results in Laundrette::configureDependencies() returning a list of two dependencies where the second entry is an empty bytes keycode.

This has some serious repercussion effects. While the MoneyShelf module is correctly registed with the system earlier via Actions.InstallModule and references to the individual modules work correctly within Laundrette, the Kernel is not aware that the Laundrette policy contract depends on a module in with the keycode MONEY. In practice, this means when the Laundrette policy has to be reconfigured due to a module upgrade, it will not be found for reconfiguration.

Impact

In case the godfather upgrades the MoneyShelf contract to the MoneyVault as described by the project, the protocol will fail to reconfigure the dependencies for the Laundrette contract as it can't find it in the dependency graph.

As a result of that, Laundrette::moneyShelf will keep a reference to the old MoneyShelf instance and the upgrade essentially failed.

Tools Used

• Manual review

• Foundry for testing

Recommended Mitigation

Ensure Laundrette::configureDependencies() returns the correct list of keycodes:

Proof of Concept

Below is a test that shows that after running the EmergencyMigration script, Laundrette::moneyShelf still points at the original MoneyShelf contract instance, instead of the new MoneyVault instance.

Here's what the storage layout of Laundrette looks like, we expect Laundrette::moneyShelf to live in storage slot 1:

This test can be dropped straight into EmergencyMigration.t.sol. Without the recommended fix, this will fail: