Summary
EmergencyMigration::migrate function correctly upgrades the MoneyShelf module to MoneyVault, however, it fails to move the USDC tokens from the MoneyShelf to the MoneyVault. This omission can lead to a situation where the funds remain in the old contract, potentially exposed to the issues the migration was meant to address.
Vulnerability Details
EmergencyMigration::migrate function is intended to transition control of USDC from the MoneyShelf contract to the MoneyVault contract in case of an emergency. However, the function only upgrades the module without transferring the actual USDC balance to the new contract. The relevant code snippet is as follows:
function migrate(Kernel kernel, IERC20 usdc, CrimeMoney crimeMoney) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
@> kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
vm.stopBroadcast();
return moneyVault;
}
Without transferring the USDC, the funds remain in the MoneyShelf, defeating the purpose of the emergency migration.
Impact
USDC remains in the vulnerable MoneyShelf contract, which may be exposed to the security issue the migration was meant to mitigate. Also, not transferring USDC to MoneyVault undermines its role as a secure fallback during emergencies, potentially leading to financial losses or operational failures.
Tools Used
Manual Inspection
Proof of Code
Place the following test into the EmergencyMigration.t.sol:
function test_migrate() public {
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
vm.startPrank(deployer.godFather());
usdc.approve(address(moneyShelf), type(uint256).max);
laundrette.depositTheCrimeMoneyInATM(deployer.godFather(), address(moneyShelf), 1_000_000e6);
vm.stopPrank();
EmergencyMigration migration = new EmergencyMigration();
uint256 balance_moneyShelf_before = usdc.balanceOf(address(moneyShelf));
MoneyVault moneyVault = migration.migrate(kernel, laundrette, usdc, crimeMoney);
uint256 balance_moneyShelf_after = usdc.balanceOf(address(moneyShelf));
uint256 balance_moneyVault_after = usdc.balanceOf(address(moneyVault));
console.log("~~~ Before ~~~");
console.log("moneyShelf - before: ", balance_moneyShelf_before);
console.log("moneyVault - before: ", 0);
console.log("~~~~~~~~~~~~~~");
console.log("~~~ After ~~~");
console.log("moneyShelf - after: ", balance_moneyShelf_after);
console.log("moneyVault - after: ", balance_moneyVault_after);
console.log("~~~~~~~~~~~~~~");
assertNotEq(address(moneyShelf), address(moneyVault));
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyVault));
assertEq(balance_moneyShelf_before, balance_moneyShelf_after);
}
Running this test yields the following output:
[PASS] test_migrate() (gas: 2338186)
~~~ Before ~~~
moneyShelf - before: 1000000000000
moneyVault - before: 0
~~~~~~~~~~~~~~
~~~ After ~~~
moneyShelf - after: 1000000000000
moneyVault - after: 0
~~~~~~~~~~~~~~
Recommendations
To resolve this issue, modify the EmergencyMigration::migrate function to include the transfer of USDC from the MoneyShelf to the MoneyVault.
function deploy() public returns (Kernel, IERC20, CrimeMoney, WeaponShelf, MoneyShelf, Laundrette) {
// Code above stays the same...
kernel.grantRole(Role.wrap("moneyshelf"), address(moneyShelf));
+ kernel.grantRole(Role.wrap("gangmember"), address(godFather)); // Add godfather to the gang
// Code below stays the same...
}
-function migrate(Kernel kernel, IERC20 usdc, CrimeMoney crimeMoney) public returns (MoneyVault) {
+function migrate(Kernel kernel, Laundrette laundrette, IERC20 usdc, CrimeMoney crimeMoney) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
+ MoneyShelf moneyShelf = MoneyShelf(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))));
+ laundrette.withdrawMoney(address(moneyShelf), address(moneyVault), usdc.balanceOf(address(moneyShelf)));
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
vm.stopBroadcast();
return moneyVault;
}
Running the test again would yield the following outcome:
[FAIL. Reason: assertion failed: 1000000000000 != 0] test_migrate() (gas: 2464336)
~~~ Before ~~~
moneyShelf - before: 1000000000000
moneyVault - before: 0
~~~~~~~~~~~~~~
~~~ After ~~~
moneyShelf - after: 0
moneyVault - after: 1000000000000
~~~~~~~~~~~~~~
