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

MoneyShelf role definition is used in multiple places, preventing accounts from minting CrimeMoney when updated inconsistently

r4bbit

Summary

There are multiple definitions of the same moneyshelf role via Roles.wrap("moneyshelf") in different places. These are used to ensure only the MoneyShelf can mint and burn CrimeMoney tokens. Having them defined in multiple places like this is error prone, as there's no compilation error when adding typos to the role ID. This could cause the protocol to not function correctly as all deposits would fail due an access control role mismatch.

Vulnerability Details

The mafia takedown contracts use the Default framework to manage things like upgrades, access control and modules of the protocol. For access control between modules, one can define custom roles via Roles.wrap(). These can then be used to check if an account has such a role, allowing for implementing access control to certain functions in contracts.

We can see this in action in a few places. In Deployer.s.sol a role is created for the MoneyShelf contract with the ID moneyshelf.

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

This is then later used in CrimeMoney::onlyMoneyShelf, a modifier that ensures that the caller of a function is always an account that has the moneyshelf role.

modifier onlyMoneyShelf() {
require(kernel.hasRole(msg.sender, Role.wrap("moneyshelf")), "CrimeMoney: only MoneyShelf can mint");
_;
}

This modifier is used on CrimeMoney::mint and CrimeMoney::burn:

function mint(address to, uint256 amount) public onlyMoneyShelf {
_mint(to, amount);
}
​
function burn(address from, uint256 amount) public onlyMoneyShelf {
_burn(from, amount);
}

If this there's a change made to one of these role definitions, either in Deployer.s.sol or in the onlyMoneyShelf modifier, this would break the protocol as the kernel.hasRole() check would always fail.

This will not be caught during compilation as the code would still compile, even if there's a typo in one of the role definitions (say, moneshelf instead of moneyshelf).

Impact

A deployment or upgrade of a module that accidentally introduces a typo in a role definition will cause parts of the protocol to no longer work as intended. In this particular case, no users will be able to deposit any funds to the protocol, because when the MoneyShelf tries to CrimeMoney::mint, it will always revert.

Tools Used

  • Manual review

  • Foundry for testing

Recommended Mitigation

To reduce the likelyhood of this happening, it's recommended to create role definitions in a single place and then reuse them where they are needed.

This way, no inconsistencies between role definitions can happen because every role ID is only ever spelled out once.

For example, we could create a src/Roles.sol file with the following contents:

+ // SPDX-License-Identifier: MIT
+ pragma solidity ^0.8.13;
+
+ import { Role } from "./Kernel.sol";
+
+ library Roles {
+ Role constant MONEYSHELF_ROLE = Role.wrap("moneyshelf");
+ }

Then, in script/Deploy.s.sol we import the Roles library and use its role definitions:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import { Script, console2 } from "lib/forge-std/src/Script.sol";
import { IERC20 } from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import { HelperConfig } from "script/HelperConfig.s.sol";
​
import { Kernel, Actions, Role } from "src/Kernel.sol";
import { CrimeMoney } from "src/CrimeMoney.sol";
​
import { WeaponShelf } from "src/modules/WeaponShelf.sol";
import { MoneyShelf } from "src/modules/MoneyShelf.sol";
​
import { Laundrette } from "src/policies/Laundrette.sol";
​
+ import { Roles } from "src/Roles.sol";
​
contract Deployer is Script {
address public godFather;
​
function run() public {
vm.startBroadcast();
deploy();
vm.stopBroadcast();
}
​
function deploy() public returns (Kernel, IERC20, CrimeMoney, WeaponShelf, MoneyShelf, Laundrette) {
godFather = msg.sender;
​
// Deploy USDC mock
HelperConfig helperConfig = new HelperConfig();
IERC20 usdc = IERC20(helperConfig.getActiveNetworkConfig().usdc);
​
Kernel kernel = new Kernel();
CrimeMoney crimeMoney = new CrimeMoney(kernel);
​
WeaponShelf weaponShelf = new WeaponShelf(kernel);
MoneyShelf moneyShelf = new MoneyShelf(kernel, usdc, crimeMoney);
Laundrette laundrette = new Laundrette(kernel);
​
- kernel.grantRole(Role.wrap("moneyshelf"), address(moneyShelf));
+ kernel.grantRole(Roles.MONEYSHELF_ROLE, address(moneyShelf));
​
kernel.executeAction(Actions.InstallModule, address(weaponShelf));
kernel.executeAction(Actions.InstallModule, address(moneyShelf));
kernel.executeAction(Actions.ActivatePolicy, address(laundrette));
kernel.executeAction(Actions.ChangeAdmin, address(laundrette));
kernel.executeAction(Actions.ChangeExecutor, godFather);
​
return (kernel, usdc, crimeMoney, weaponShelf, moneyShelf, laundrette);
}
}
​

We then do the same for the kernel.hasRole() check in CrimeMoney:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
​
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import { Kernel, Actions, Role } from "./Kernel.sol";
​
+ import { Roles } from "./Roles.sol";
​
contract CrimeMoney is ERC20 {
Kernel public kernel;
​
constructor(Kernel _kernel) ERC20("CrimeMoney", "CRIME") {
kernel = _kernel;
}
​
modifier onlyMoneyShelf() {
- require(kernel.hasRole(msg.sender, Role.wrap("moneyshelf")), "CrimeMoney: only MoneyShelf can mint");
+ require(kernel.hasRole(msg.sender, Roles.MONEYSHELF_ROLE), "CrimeMoney: only MoneyShelf can mint");
_;
}
​
function mint(address to, uint256 amount) public onlyMoneyShelf {
_mint(to, amount);
}
​
function burn(address from, uint256 amount) public onlyMoneyShelf {
_burn(from, amount);
}
}

Proof of Concept

The risk mentioned in this report can be demonstrated by simply adding a typo to any of the existing role defitions that are used in multiple places.

For example, below we're introducing a typo to the kernel.hasRole() check in CrimeMoney:

modifier onlyMoneyShelf() {
- require(kernel.hasRole(msg.sender, Role.wrap("moneyshelf")), "CrimeMoney: only MoneyShelf can mint");
+ require(kernel.hasRole(msg.sender, Role.wrap("moneyself")), "CrimeMoney: only MoneyShelf can mint");
_;
}

All tests that call CrimeMoney::mint and CrimeMoney::burn will revert.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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