First Flight #16: Mafia Takedown
First Flight #16
Beginner FriendlyDeFiFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

EmergencyMigration leaves all the USDC behind.

mattjenkins

Summary

When emergency migrating the underlying MoneyShelf to the MoneyVault, in a rather unfortunate twist, the USDC is left behind in the MoneyShelf contract and not moved.

Vulnerability Details

Being good at extortion is not traditionally a correlated indicator that one is good at coding blockchain frameworks. There is a lack of understanding of the underlying Default framework for managing modules. Not only does the code-base not rely on interfaces for modules and interchangeable contracts throughout the codebase, it also doesn't have a mechanism to do any of the moving activities of assets from old module to new module, and doesn't even leverage the existing INIT function for the new MoneyVault module.

Impact

When doing the EmergencyMigration all the USDC associated with MoneyShelf is left with MoneyShelf, leaving the GodFather unable to remove the USDC that isn't there in the MoneyVault. This is only really the start of the woes for our soon to be very angry GodFather.

A test has been added to demonstrate this

// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
​
import { BaseTest, console } from "test/Base.t.sol";
import { Role, Keycode } from "src/Kernel.sol";
​
import { EmergencyMigration } from "script/EmergencyMigration.s.sol";
import { MoneyShelf } from "src/modules/MoneyShelf.sol";
import { MoneyVault } from "src/modules/MoneyVault.sol";
​
contract EmergencyMigrationTest is BaseTest {
​
function test_EmergencyMigration() public {
joinGang(address(this));
​
// Set up our gangmember and give him some USDC
vm.startPrank(godFather);
usdc.transfer(address(gangmember1), 10e6);
laundrette.addToTheGang(gangmember1);
vm.stopPrank();
​
// Our trusting Gangmember deposits his USDC into the ATM
vm.startPrank(gangmember1);
usdc.approve(address(moneyShelf), 10e6);
laundrette.depositTheCrimeMoneyInATM(gangmember1, gangmember1, 10e6);
vm.stopPrank();
​
// Check we are still using MoneyShelf
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
​
// Raid time. Godfather pulls the plug.
EmergencyMigration migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
​
// Check we are now using MoneyVault
assertNotEq(address(moneyShelf), address(moneyVault));
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyVault));
​
// Check to see where the USDC actually is
assertEq(usdc.balanceOf(address(moneyVault)), 10e6);
assertEq(usdc.balanceOf(address(moneyShelf)), 0);
}
​
}
​

Tools Used

Recommendations

When modules are upgraded, the internal pointers are all updated and in theory all relevant Policies notified. What is needed is a mechanism to transfer all the assets stored in the old module to the new one (and metadata if that was needed). A few other changes are needed to fully fix this but the main one is the USDC. Much as I wouldn't want to mess with the Default framework, it does seem to lack a Module TERMINATE(address _newModule) function which would allow an old module to transfer any assets to its upgrade. This could be then called in the Kernel _upgradeModule function alongside the INIT() of the new function, all gated with onlyKernel()

Updates

Lead Judging Commences

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

Emergency migration leave the USDC

Support

FAQs

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