Summary
All funds deposited into the MoneyShelf contract via Laundrette::depositTheCrimeMoneyInATM are at risk to be lost because there's no mechanism that ensures these funds are migrated to the MoneyVault contract when an emergency upgrade is done.
Vulnerability Details
The mafia takedown contracts allow any users to deposit USDC into a MoneyShelf contract via Laundrette::depositTheCrimeMoneyInATM in return for CrimeMoney tokens. The CrimeMoney tokens can later be redeemed via Laundrette::withdrawMoney to get back the USDC tokens.
In case of an emergency, the godfather of the protocol can run an EmergencyMigration which replaces the MoneyShelf contract with a MoneyVault contract:
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;
}
The idea is that, once upgraded, nobody can deposit funds into the protocol anymore and only the godfather can withdraw all locked up USDC on behalf of all depositors:
function depositUSDC(address, address, uint256) external pure {
revert("MoneyVault: depositUSDC is disabled");
}
function withdrawUSDC(address account, address to, uint256 amount) external {
require(to == kernel.executor(), "MoneyVault: only GodFather can receive USDC");
withdraw(account, amount);
crimeMoney.burn(account, amount);
usdc.transfer(to, amount);
}
However, there's a serious bug: the upgrade strategy doesn't take into account that the funds that live in the MoneyShelf contract need to be transferred to the MoneyVault contract (before) the upgrade is done.
This means, when the godfather runs the emergency migration, the module will be replaced and all funds will be locked in the old MoneyShelf instance forever as there's no way to retrieve them again.
Impact
After performing the emergency migration, or any other migration that upgrades the MoneyShelf module with something else, all funds deposited by users will be lost.
Tools Used
Manual review
Foundry for testing
Recommended Mitigation
If the framework that is used (Default in this case) doesn't have a solution for running something like hooks, before and after a module is upgraded, the only way to ensure funds are not lost is to either never do an emergency upgrade, or extend the MoneyShelf contract with a function to migrate the funds to the new module.
A very rudimentary version of that could look like this:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { CrimeMoney } from "src/CrimeMoney.sol";
import { Kernel, Keycode } from "src/Kernel.sol";
import { Shelf } from "src/modules/Shelf.sol";
contract MoneyShelf is Shelf {
IERC20 private usdc;
CrimeMoney private crimeMoney;
constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney) Shelf(kernel_) {
usdc = _usdc;
crimeMoney = _crimeMoney;
}
function KEYCODE() public pure override returns (Keycode) {
return Keycode.wrap("MONEY");
}
// "permissioned" already used by Shelf functions
function depositUSDC(address account, address to, uint256 amount) external {
deposit(to, amount);
usdc.transferFrom(account, address(this), amount);
crimeMoney.mint(to, amount);
}
function withdrawUSDC(address account, address to, uint256 amount) external {
withdraw(account, amount);
crimeMoney.burn(account, amount);
usdc.transfer(to, amount);
}
+ function migrateFunds(address account) external {
+ require(msg.sender == kernel.executor(), "MoneyShelf: only GodFather can migrate funds");
+ usdc.transfer(account, usdc.balanceOf(address(this)));
+ }
}
Obviously special care has to be taken here. There's a new risk that the executor could send all the funds to any other account, resulting in the loss of all funds as well.
However, since there's already centralization risk and the executor is trusted, this might be a solution worth considering.
We can then change the EmergencyMigration script accordingly:
function migrate(Kernel kernel, IERC20 usdc, CrimeMoney crimeMoney) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
+ // this has to happen before upgrading the module
+ address moneyShelf = address(kernel.getModuleForKeycode(Keycode.wrap("MONEY")));
+ MoneyShelf(moneyShelf).migrateFunds(address(moneyVault));
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
vm.stopBroadcast();
// Once the problem is solved, GodFather migrate to a new contract and redistribute manually
// all the money to gang members thanks to event monitoring and his accountant.
return moneyVault;
}
With these changes, the test case in the proof of concept shown below should fail.
Proof of Concept
To proof this bug with a test case, we first have to address another issue that lives in this protocol and has been covered in another submission:
The dependency configuration done in Laundrette::configureDependencies returns an incorrect list of keycodes. We need to assume this has been fixed, otherwise we'll get false positives.
To fix this issue, apply the following changes in Laundrette::configureDependencies:
function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](2);
dependencies[0] = toKeycode("MONEY");
moneyShelf = MoneyShelf(getModuleAddress(toKeycode("MONEY")));
- dependencies[0] = toKeycode("WEAPN");
+ dependencies[1] = toKeycode("WEAPN");
weaponShelf = WeaponShelf(getModuleAddress(toKeycode("WEAPN")));
}
Once that is done, we can drop these changes into EmergencyMigration.t.sol to confirm the bug:
+ function test_DepositsNotTransferredBug() public {
+ // `MockUSDC` mints `1_000_000e6` token to `godFather` in tests
+ // this is our starting point
+ assertEq(usdc.totalSupply(), 1000000e6);
+ assertEq(usdc.balanceOf(godFather), 1000000e6);
+
+ // create two arbitrary users
+ address userOne = makeAddr("userOne");
+ address userTwo = makeAddr("userTwo");
+
+ // ensure they have some tokens to deposit
+ vm.startPrank(godFather);
+ usdc.transfer(userOne, 100e6);
+ usdc.transfer(userTwo, 100e6);
+ vm.stopPrank();
+
+ // double check that balances have updated correctly
+ assertEq(usdc.balanceOf(userOne), 100e6);
+ assertEq(usdc.balanceOf(userTwo), 100e6);
+ assertEq(usdc.balanceOf(godFather), 1000000e6-200e6);
+
+ // both users approve `moneyShelf` to spend their tokens
+ vm.prank(userOne);
+ usdc.approve(address(moneyShelf), 100e6);
+ vm.prank(userTwo);
+ usdc.approve(address(moneyShelf), 100e6);
+
+ // then both users deposit their tokens
+ vm.prank(userOne);
+ laundrette.depositTheCrimeMoneyInATM(userOne, userOne, 100e6);
+ vm.prank(userTwo);
+ laundrette.depositTheCrimeMoneyInATM(userTwo, userTwo, 100e6);
+
+ // ensuring funds ended up in `MoneyShelf` and users have minted `CrimeMoney`
+ assertEq(crimeMoney.balanceOf(userOne), 100e6);
+ assertEq(crimeMoney.balanceOf(userTwo), 100e6);
+ assertEq(usdc.balanceOf(userOne), 0);
+ assertEq(usdc.balanceOf(userTwo), 0);
+ assertEq(usdc.balanceOf(address(moneyShelf)), 200e6);
+ assertEq(usdc.balanceOf(godFather), 1000000e6-200e6);
+
+ // ensure `godFather` has `gangmember` role, otherwise can't withdraw funds
+ vm.prank(address(laundrette));
+ kernel.grantRole(Role.wrap("gangmember"), godFather);
+
+ // next, assume the `EmergencyMigration` needs to be performed
+ // `moneyVault` is now the new module that's used for depositing/withdrawing funds
+
+ EmergencyMigration migration = new EmergencyMigration();
+ MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
+ // `godFather` tries to withdraw funds from `MoneyVault` however,
+ // since the funds have never been transferred from `MoneyShelf`,
+ // there's nothing to withdraw - this will revert with an arithmetic underflow
+ vm.startPrank(godFather);
+ vm.expectRevert();
+ laundrette.withdrawMoney(userOne, godFather, 100e6);
+
+ // check that `moneyVault` has indeed no funds
+ // assertEq(usdc.balanceOf(address(moneyVault)), 0);
+ assertEq(usdc.balanceOf(address(moneyVault)), 200e6);
+ }
