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

GodFather unable to remove USDC from MoneyVault

mattjenkins

Summary

If the patron saint of mobsters is feeling kind and by some miracle there is all the USDC in the MoneyVault, the GodFather faces yet more challenges. The withdrawal of funds from the Vault is predicated on the logic within the withdrawUSDC function, and sadly, this is identical to the MoneyShelf contract which checks the bank mapping balance for the calling account as well as requiring the burning of CrimeTokens, neither of which happens to have favourable values for the GodFather.

Vulnerability Details

The MoneyVault contract has the same logic as the MoneyShelf when it comes to withdrawals. It burns an equivalent amount of CrimeTokens to release the USDC, as well as reducing the balance stored. Given the balance isn't even brought over, this is set to zero for the GodFather in the MoneyVault. This means the amount of USDC released is rather limited. Every attempt to remove any USDC that miraculously works its way into MoneyVault will result in the inevitable panic: arithmetic underflow or overflow errors.

Impact

Severe. The USDC is locked up without the correct balance in the bank mapping and the right amount of CrimeTokens.

Tools Used

forge test to demonstrate (along with some help as this situation requires massaging to get to the situation where there is actual USDC in MoneyVault.

// This assumes we have fixed the laundrette dependency issue somehow
function test_EmergencyMigrationSuccess() public {
joinGang(address(this));
​
// Godfather deposits his USDC into the ATM
vm.startPrank(godFather);
usdc.approve(address(moneyShelf), 1e6);
laundrette.depositTheCrimeMoneyInATM(godFather, godFather, 1e6);
vm.stopPrank();
​
// Check we are still using MoneyShelf
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
​
// Raid time. Godfather pulls the plug.
EmergencyMigration migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
​
// Check we are now using MoneyVault
assertNotEq(address(moneyShelf), address(moneyVault));
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyVault));
​
// Check to see where the USDC actually is
assertEq(usdc.balanceOf(address(moneyVault)), 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 1e6);
​
// Now massage the numbers to make it look like the migration was successful
vm.startPrank(godFather);
usdc.transfer(address(moneyVault), 10e6);
vm.stopPrank();
​
assertEq(usdc.balanceOf(address(moneyVault)), 10e6);
console.log("Godfather balance of USDC: ", usdc.balanceOf(address(godFather))); //999989000000
​
// try and get USDC out of the vault
vm.startPrank(godFather);
laundrette.withdrawMoney(godFather, godFather, 1e6);
​
}

Recommendations

Turn yourself in. Your developers are either corrupt, stupid or hate you.

However if the GodFather has some coding-fu, then they could deploy a new MoneyVault contract with a proper init which removes the withdraw function and CrimeMoney.burn function in withdrawUSDC.
To pay homage and have at least some consistency with the framework itself, a permissioned modifier should be added. There is a check for the executor already, and in this situation, engineering niceness is probably not high on the list of priorities but it would show a degree of class hitherto not evident within this organisation.

- function withdrawUSDC(address account, address to, uint256 amount) external {
+ 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);
}
Updates

Lead Judging Commences

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

Transfer CrimeMoney break the protocol/bad account tracking

Support

FAQs

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