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

[H-3] Godfather can not withdraw funds from the `MoneyVault` contract after `EmergencyMigration`.

Keyword

Description: The EmergencyMigration::migrate function is supposed to change the MoneyShelf contract to a MoneyVault contract, and erase the traces of the old Kernel, USDC and CrimeMoney addresses by setting them to address(0).

The MoneyVault contract does not allow anyone to deposit funds in it, and only the address of Godfather, kernel.executor() can be set as the receiver of the withdrawals. However, the old Module contracts are not compatible with the new Policy contract and the funds will be stuck in the MoneyVault contract forever. When taking into consideration that the new Kernel address is set to address(0) during migration, this will make it impossible to add the new contract in the old Kernel after migration.

Impact: No one can withdraw funds from the protocol anymore, not even Capo di tutti capi.

Proof of Concepts: Place the following test into the EmergencyMigration.t.sol file.

PoC - click the arrow below
// change imports
import { Kernel, Policy, Permissions, Keycode, Role, Actions } from "src/Kernel.sol";
​
function test_migrateAndWithdraw() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
vm.startPrank(godFather);
usdc.transfer(alice, 100e6);
usdc.transfer(bob, 100e6);
vm.stopPrank();
​
vm.prank(alice);
usdc.approve(address(moneyShelf), 100e6);
vm.prank(bob);
usdc.approve(address(moneyShelf), 100e6);
​
vm.prank(alice);
laundrette.depositTheCrimeMoneyInATM(alice, alice, 100e6);
vm.prank(bob);
laundrette.depositTheCrimeMoneyInATM(bob, bob, 100e6);
​
assertEq(usdc.balanceOf(address(moneyShelf)), 200e6);
​
// Emergency Migration starts here
​
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
​
EmergencyMigration migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
​
assertNotEq(address(moneyShelf), address(moneyVault));
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyVault));
​
console.log(usdc.balanceOf(address(moneyVault)), 200e6);
​
// Try to withdraw after migration
​
vm.prank(alice);
moneyVault.withdrawUSDC(alice, kernel.executor(), 100e6);
}

Test output

├─ [5218] MoneyVault::withdrawUSDC(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], God Father: [0xe166Ae83c3384a19498Ae0674706988DD2797489], 100000000 [1e8])
│ ├─ [393] Kernel::executor() [staticcall]
│ │ └─ ← [Return] God Father: [0xe166Ae83c3384a19498Ae0674706988DD2797489]
│ ├─ [2917] Kernel::modulePermissions(0x4d4f4e4559000000000000000000000000000000000000000000000000000000, EmergencyMigrationTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x7922620900000000000000000000000000000000000000000000000000000000) [staticcall]
@> │ │ └─ ← [Return] false
@> │ └─ ← [Revert] Module_PolicyNotAuthorized(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)
@>└─ ← [Revert] Module_PolicyNotAuthorized(0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496)
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.62ms (893.90µs CPU time)

Recommended mitigation: For backward compatibility purposes, the new MoneyVault contract has to be granted permission to access the old Module contracts during migration. In doing so, the mob won't be able to clear their traces. The logic here needs to be rewritten.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Keyword Submitter
about 1 year ago
n0kto Lead Judge
about 1 year ago
n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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