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

Funds doesn't move from `MoneyShelf` to `MoneyVault` after `EmergencyMigration` causing the assets be unprotected

Mill

Summary

According to the Documentation:
"In case of any issue (on-chain or off-chain), MoneyShelf is updated to this contract (MoneyVault) to protect the money from the justice system or any other gang. Only the GodFather can withdraw and no one can deposit in this contract."

Vulnerability Details

The funds are not transferred to the MoneyVault contract, leading to unprotected assets inside the MoneyShelf contract.

Proof of Code

Paste the following code inside the EmergencyMigrationTest file, then run the script:

Code
function test_fundsDoesntMoveAfterMigration() public {
uint256 startingBalance = 100e6;
uint256 depositBalance = 50e6;
​
vm.startPrank(godFather);
usdc.transfer(testAccount, startingBalance);
vm.stopPrank();
​
vm.startPrank(testAccount);
usdc.approve(address(moneyShelf), startingBalance);
laundrette.depositTheCrimeMoneyInATM(testAccount, testAccount, depositBalance);
vm.stopPrank();
​
assertEq(usdc.balanceOf(address(moneyShelf)), depositBalance);
assertEq(usdc.balanceOf(testAccount), startingBalance - depositBalance);
​
// We do a migration
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));
​
// The moneyVault has no balance
assertEq(usdc.balanceOf(address(moneyVault)), 0);
// The moneyShelf still holds the money
assertEq(usdc.balanceOf(address(moneyShelf)), depositBalance);
}
forge test --mt test_fundsDoesntMoveAfterMigration -vvvv

Impact

This design flaw leads to unprotected assets inside the MoneyShelf contract, because other gang members could still withdraw their USDC from the MoneyShelf contract. This breaks the protocols invariant, that only the GodFather should be able to withdraw after a migration.

Tools Used

  • forge test

Recommendations

  1. Implemented a new function in the MoneyShelf contract that allows the Executor (GodFather) to send the total USDC balance of MoneyShelf to a new address (in this case the MoneyVault).

function migrateFundsToVault(address to) external {
require(kernel.executor() == msg.sender, "MoneyShelf: you are not the godfather");
uint256 totalUsdcBalance = usdc.balanceOf(address(this));
usdc.transfer(to, totalUsdcBalance);
}

  1. Update the EmergencyMigration script contract :

+ import { MoneyShelf } from "src/modules/MoneyShelf.sol";
​
contract EmergencyMigration is Script {
address public godFather;
​
function run() public {
// To set at the deployment, no leak of addresses before.
Kernel kernel = Kernel(address(0));
IERC20 usdc = IERC20(address(0));
CrimeMoney crimeMoney = CrimeMoney(address(0));
+ MoneyShelf moneyShelf = MoneyShelf(address(0));
​
- migrate(kernel, usdc, crimeMoney);
+ migrate(kernel, usdc, crimeMoney, moneyShelf);
}
​
// In case of any issue, the GodFather call this function to migrate the money shelf to a contract that only him
// can manage the money.
// This function has to success in any case to protect the money.
- function migrate(Kernel kernel, IERC20 usdc, CrimeMoney crimeMoney) public returns (MoneyVault) {
+ function migrate(Kernel kernel, IERC20 usdc, CrimeMoney crimeMoney, MoneyShelf moneyShelf) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
+ moneyShelf.migrateFundsToVault(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;
}
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Emergency migration leave the USDC

Support

FAQs

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