Description
In the migration script EmergencyMigration.s.sol, a new contract MoneyVault is created, but state varibles mapping(address => uint256) bank is not migrated from MoneyShelf to MoneyVault. Nor are the USDC tokens transferred to MoneyVault.
Impact
After migration, the USDC token still stays in the MoneyShelf, and cannot be fetched, unless the module is upgraded back to MoneyShelf. Also function Laundrette::withdrawMoney wont work, because bank mapping is empty in MoneyShelf. This leads to the situation that nobody can retrieve the USDC deposited.
Proof of Concept
function test_migrateAndWithdraw() public {
address alice = makeAddr("alice");
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
joinGangGodFather();
joinGang(address(this));
vm.prank(godFather);
usdc.transfer(alice, 100e6);
vm.startPrank(alice);
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(alice, alice, 100e6);
vm.stopPrank();
assertEq(usdc.balanceOf(alice), 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 100e6);
assertEq(crimeMoney.balanceOf(alice), 100e6);
EmergencyMigration migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
assertEq(usdc.balanceOf(address(moneyVault)), 0);
vm.prank(godFather);
vm.expectRevert();
laundrette.withdrawMoney(alice, godFather, 100e6);
}
Recommendations
Directly copying the whole mapping from MoneyShelf to MoneyVault is too expensive. One way to do it is to seperate the storage part into another storage contract. But, this requires a lot of changes. Another easier way is to remove the bank mapping and only rely on crimeMoney token balance. for example:
in MoneyShelf:
-- contract MoneyShelf is Shelf {
++ contract MoneyShelf is Module {
IERC20 private usdc;
CrimeMoney private crimeMoney;
-- constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney) Shelf(kernel_) {
++ constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney) Module(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 permissioned{
-- deposit(to, amount);
usdc.transferFrom(account, address(this), amount);
crimeMoney.mint(to, amount);
}
function withdrawUSDC(address account, address to, uint256 amount) external permissioned {
-- withdraw(account, amount);
crimeMoney.burn(account, amount);
usdc.transfer(to, amount);
}
++ function migrate() external {
++ require(kernel.hasRole(msg.sender, Role.wrap("moneyshelf")), "MoneyShelf: only migrate to MoneyShelf Module");
++ usdc.transfer(msg.sender, usdc.balanceOf(address(this)));
++ }
}
in MoneyVault:
-- contract MoneyVault is Shelf {
++ contract MoneyVault is Module {
IERC20 private usdc;
CrimeMoney private crimeMoney;
++ MoneyShelf private oldMoneyShelf;
-- constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney) Shelf(kernel_) {
++ constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney, MoneyShelf _oldMoneyShelf) Module(kernel_) {
usdc = _usdc;
crimeMoney = _crimeMoney;
++ oldMoneyShelf = _oldMoneyShelf;
}
function KEYCODE() public pure override returns (Keycode) {
return Keycode.wrap("MONEY");
}
function depositUSDC(address, address, uint256) external pure {
revert("MoneyVault: depositUSDC is disabled");
}
function withdrawUSDC(address account, address to, uint256 amount) external permissioned {
require(to == kernel.executor(), "MoneyVault: only GodFather can receive USDC");
-- withdraw(account, amount);
crimeMoney.burn(account, amount);
usdc.transfer(to, amount);
}
++ function INIT() external override onlyKernel {
++ oldMoneyShelf.migrate();
++ }
}
