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

`MoneyVault` lacks `moneyShelf` role, `MoneyVault::withdrawUSDC` will fail

paprikrumplikas

Summary

EmergencyMigration misses to assign the moneyShelf role to MoneyVault.
Without the role, MoneyVault will not be able to call CrimeMoney::burn (or CrimeMoney::mint) and, conseqently, USDC withdrawal from MoneyVault will fail.

Consider MoneyVault::withdrawUSDC which requires MoneyVault to be able to call CrimeMoney::burn` for successful execution:

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);
}

The following test demonstrates that

  • MoneyVault does not have the moneyShelf role after migration, and that

  • without this role, a call to MoneyVault::withdrawUSDC will revert with MoneyVault: only GodFather can receive USDC:

Note! For this test to properly work, uncomment the following line in MoneyVault, as that is another bug:

withdraw(account, amount);
Proof of Code
function testMoneyVaultIsNotAssignedMoneyShelfRoleCantWithdraw() public {
////////////////////////////////////////////////
/// Emergency migration ////////////////////////
////////////////////////////////////////////////
​
EmergencyMigration migration;
​
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
​
migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
​
// mitigating the effects of another bug
vm.startPrank(godFather);
kernel.executeAction(Actions.DeactivatePolicy, address(laundrette));
kernel.executeAction(Actions.ActivatePolicy, address(laundrette));
vm.stopPrank();
​
// MoneyVault does not have moneyShelf role
assertEq(kernel.hasRole(address(moneyVault), Role.wrap("moneyshelf")), false);
​
//////////////////////////////////////////////////
/////// SETUP OF BALANCES ////////////////////////
//////////////////////////////////////////////////
​
// suppose USDC funds have been transferred to moneyVault
uint256 savedFunds = 1e6; // i.e. total supply of crimeMoney
deal(address(usdc), address(moneyVault), savedFunds);
​
// suppose godfather recovered crimeMoney for withdrawal
uint256 crimeFunds = savedFunds;
deal(address(crimeMoney), godFather, crimeFunds);
​
//////////////////////////////////////////////////
/////// GODFATHER TRIES TO WITHDRAW USDC /////////
//////////////////////////////////////////////////
​
// correcting another bug: giving the godfather gangmember role
vm.prank(kernel.admin());
kernel.grantRole(Role.wrap("gangmember"), godFather);
​
vm.startPrank(godFather);
vm.expectRevert("CrimeMoney: only MoneyShelf can mint");
laundrette.withdrawMoney(godFather, godFather, savedFunds);
vm.stopPrank();
}

Impact

While MoneyVault lacks the moneyShelf role, all USDC funds of the Mafia will be stuck in MoneyVault.

Luckily, the godfather can mitigate the impact by performing the following steps:

  • Reclaim the admin rights to Kernel by executing

kernel.executeAction(Actions.ChangeAdmin, godFather);

Note, however, that while he is the admin, Laundrette::addToTheGang and Laundrette::quitTheGang will not work.

  • Grant the moneyShelf role to the deployed instance of MoneyVault by executing

kernel.grantRole(Role.wrap("moneyshelf"), address(moneyVault));

  • (and then he should give back the admin role to Laundrette by executing)

kernel.executeAction(Actions.ChangeAdmin, address(laundrette));

Tools Used

Manual review, Foundry.

Recommendations

The easiest solution is to deploy the MoneyVault contract together with the other contracts, in Deploy.s.sol, and grant MoneyVault the moneyShelf role while the godfather is still the admin of Kernel.

Alternatively, modify EmergencyMigration so that during the migration the moneyShelf role is granted to MoneyVault.

Updates

Lead Judging Commences

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

MoneyVault cannot burn or mint CrimeMoney

Godfather can add the role manually

Support

FAQs

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