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

The MoneyVault::withdrawUSDC function fails on call to CrimeMoney::burn preventing the GodFather from withdrawing USDC

poolig4n

Description
When the Godfather tries to withdraw USDC from the MoneyVault the call to CrimeMoney::burnwill fail due to the modifier on the burn function requiring the moneyshelf role.

Impact

Funds cannot be withdrawn from the MoneyVault by the GodFather which contradicts the documentation:

MoneyVault:
Only the GodFather can withdraw and no one can deposit in this contract.

Proof of Concept

File: MoneyVault.sol
Reformatted for clarity:

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); <- This line will fail
usdc.transfer(to, amount);
}

The call to crimeMoney.burn(account, amount); will fail, this will cause a revert; the withdraw(account, amount) will be unwound and the transfer will not be executed.

As shown below, the moneyshelf role is required to call the burn function.
File: CrimeMoney.sol
Reformatted for clarity:

modifier onlyMoneyShelf() {
require(kernel.hasRole(msg.sender,
Role.wrap("moneyshelf")),
"CrimeMoney: only MoneyShelf can mint");
_;
}
function burn(address from, uint256 amount) public onlyMoneyShelf {
_burn(from, amount);
}
​

File: CrimeMoney.sol
Reformatted for clarity and comments removed

function migrate(Kernel kernel, IERC20 usdc,
CrimeMoney crimeMoney) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
vm.stopBroadcast();
return moneyVault;
}

As you can see in the code snippets above the migrate function does not grant the moneyshelf role to the moneyVault address, and the burn function uses the modifier onlyMoneyShelf.

Recommended mitigation
Note: This code is indicative of one possible solution, it must not be considered production ready code.

Add a new modifier to support MoneyVault calling CrimeMoney::burn.

File: CrimeMoney.sol

modifier onlyMoneyShelfOrMoneyVault() {
moneyShelfOrVault = kernel.hasRole(msg.sender,
Role.wrap("moneyshelf") ||
kernel.hasRole(msg.sender,
Role.wrap("moneyvault");
require(moneyShelfOrVault),
"CrimeMoney: only the MoneyShelf or MoneyVault can burn");
_;
}

Update the CrimeMoney::burn function to use the new modifier
File: CrimeMoney.sol

- function burn(address from, uint256 amount) public onlyMoneyShelf {
+ function burn(address from, uint256 amount) public onlyMoneyShelfOrMoneyVault {
_burn(from, amount);
}
​

Grant the new moneyvault role to the MoneyVault in the EmergencyMigration script.
File: EmergencyMigration.s.sol

function migrate(Kernel kernel, IERC20 usdc,
CrimeMoney crimeMoney) public returns (MoneyVault) {
vm.startBroadcast(kernel.executor());
MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
​
+ // MoneyVault needs to be granted the role to burn CrimeMoney.
+ kernel.grantRole(Role.wrap("moneyvault"), address(moneyVault));
vm.stopBroadcast();
return moneyVault;
}
​

References
N/A

Tools Used

  • Manual Review

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.