[H-2] Deposited USDC Stuck in MoneyShelf in Emergency Mode
Description:
Note: This only happens if the bug in the Laundrette::configureDependencies is fixed.
function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](2);
dependencies[0] = toKeycode("MONEY");
moneyShelf = MoneyShelf(getModuleAddress(toKeycode("MONEY")));
@> dependencies[1] = toKeycode("WEAPN");
weaponShelf = WeaponShelf(getModuleAddress(toKeycode("WEAPN")));
}
Upon addressing the bug in configureDependencies, a new issue arises where the MoneyShelf module fails to transfer deposited USDC to the newly upgraded MoneyVault during emergency mode. Additionally, the bank mapping is not updated accordingly, leaving the funds trapped within MoneyShelf.
Impact:
Although this issue does not lead to permanent fund loss, and is fixable by just executing UpgradeModule and replacing the MoneyVault with MoneyShelf again. it severely impacts the functionality of Emergency Mode, rendering funds inaccessible to all actors involved.
Proof of Concept:
To prove the concept, add the following test to EmergencyMigration.t.sol, Exploit Steps:
gangM1 joins gang.
gangM1 deposits some USDC to test it after emergency.
godfather enters emergency mode via calling migrate.
gangM1 Can Not withdraw because in emergency mode and previous bug is fixed.
godfather Can Not withdraw because the funds are in Moneyshell and the bank mapping is not updated.
function testMoenyIsStuckInMoneyShelf() public {
address gangM1 = makeAddr("M1");
joinGang(gangM1);
vm.prank(godFather);
usdc.transfer(gangM1, 100e6);
vm.startPrank(gangM1);
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(gangM1, gangM1, 100e6);
vm.stopPrank();
assertEq(usdc.balanceOf(gangM1), 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 100e6);
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));
vm.startPrank(gangM1);
vm.expectRevert("MoneyVault: only GodFather can receive USDC");
laundrette.withdrawMoney(gangM1, gangM1, 100e6);
uint256 gfBalanceBefore = usdc.balanceOf(godFather);
vm.startPrank(godFather);
vm.expectRevert();
laundrette.withdrawMoney(gangM1, godFather, 100e6);
assertEq(usdc.balanceOf(godFather) - gfBalanceBefore, 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 100e6);
assertEq(usdc.balanceOf(address(moneyVault)), 0);
}
Recommended Mitigation:
Introduce an emergencyMode function within MoneyShelf. This function should facilitate the transfer of funds to the new MoneyVault whenever there's a change in the MONEY keycode. Modify Laundrette to invoke this function when necessary. Furthermore, directly update the bank mapping within MoneyVault::withdrawUSDC to circumvent the issue.
Recommended Mitigation Steps
First, implement the emergencyMode function in MoneyShelf:
+ function emergencyMode(address _newVault) external permissioned {
+ usdc.transfer(_newVault, usdc.balanceOf(address(this)));
+ }
Next, adjust Laundrette::configureDependencies and Laundrette::requestPermissions to utilize this new function:
function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](2);
dependencies[0] = toKeycode("MONEY");
+ if (address(moneyShelf) != address(0)) {
+ address MoneyVault = getModuleAddress(toKeycode("MONEY"));
+ moneyShelf.emergencyMode(MoneyVault);
+ }
moneyShelf = MoneyShelf(getModuleAddress(toKeycode("MONEY")));
dependencies[1] = toKeycode("WEAPN");
weaponShelf = WeaponShelf(getModuleAddress(toKeycode("WEAPN")));
}
function requestPermissions() external view override onlyKernel returns (Permissions[] memory requests) {
- requests = new Permissions[](4);
+ requests = new Permissions[](5);
requests[0] = Permissions(toKeycode("MONEY"), moneyShelf.depositUSDC.selector);
requests[1] = Permissions(toKeycode("MONEY"), moneyShelf.withdrawUSDC.selector);
requests[2] = Permissions(toKeycode("WEAPN"), weaponShelf.deposit.selector);
requests[3] = Permissions(toKeycode("WEAPN"), weaponShelf.withdraw.selector);
+ requests[4] = Permissions(toKeycode("MONEY"), moneyShelf.emergencyMode.selector);
}
Finally, modify MoneyVault::withdrawUSDC to bypass the need for updating the bank mapping and burning tokens through crimeMoney:
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);
}
These adjustments ensure that the GodFather remains the sole authority for withdrawing funds during emergencies, but it will cause the loss of monetary value associated with crimeMoney.
