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

Not following Default FrameWork Architecture during Emergency Migration leads to critical issues

codexbugmenot

Description

Emergency migration is a process in the protocol which migrates MoneyShelf to MoneyVault in case of any emergency which should respect the Default Framework Architecture as described in the image below

DefaultFrameworkArchitecture.png

Each Policy (contracts like Laundrette.sol) have their own modules (like MoneyShelf.sol) with kernel.sol overseeing the Policies(Front-Facing Contracts) and modules(Internal-facing contracts)

In the time of an emergency when EmergencyMigration.s.sol is deployed and migrate function is called the MoneyShelf is migrated to MoneyVault but the underlying problem is that in the Front-Facing Laundrette.sol no update is made regarding MoneyVault and the state of contract MoneyShelf.sol is never Migrated to MoneyVault.sol.

An alternate way to install a new module like MoneyVault would be to create a new Policy but none was created which may lead to complete loss of control over funds during Emergency Migration

Impact

The current EmergencyMigration.s.sol is an incorrect implementation of EmergencyMigration and it may lead to :

  1. MoneyVault being empty

  2. godFather looses control over MoneyVault since there is no policy to call its functions

  3. All the Funds being stuck in MoneyShelf with no way to retrieve it during Emergency period

Proof of Concept

Even after migration the Laundrette.sol points to the MoneyShelf.sol as its module and MoneyVault.sol's state is never updated using the Module::INIT() functions and it has no policy to call its functions which can lead to complete loss of control over funds .

POC: Improper Migration
function test_ImproperMigration() public {
// we deposit 100e6 usdc in the beginning to moneyshelf
vm.prank(godFather);
usdc.transfer(address(this), 100e6);
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(address(this), address(this), 100e6);
assertEq(usdc.balanceOf(address(this)), 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 100e6);
assertEq(crimeMoney.balanceOf(address(this)), 100e6);
​
// Migration occurs
EmergencyMigration migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney);
​
// This should revert since deposits revert in MoneyVault but that is not the case
// proving that laundrette doesn't point to MoneyVault
vm.prank(godFather);
usdc.transfer(address(this), 100e6);
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(address(this), address(this), 100e6);
​
// The state of MoneyShelf isn't transferred to MoneyVault
assertEq(usdc.balanceOf(address(moneyVault)), 0);
}
​

Recommended Mitigation

A mitigation to consider would be to just DeactivatePolicy pointing to address of Laundrette.sol and ActivatePolicy(re-activate) it again this makes the Laundrette.sol point to MoneyVault.sol but the state of MoneyVault will still be empty as it isn't updated anywhere and requires to be addresses and also the Laundrette.sol has many other functions which are unaccounted for after migration

hence a better solution would be to introduce a new Policy for emergency migration named Emergency.sol,this respects the Default Frame Work Architecture as well as the design needs of the protocol.

Regarding the state migration the actual funds and the details of balances are stored in MoneyShelf.sol which are needed to be migrated to MoneyVault.sol when the module is upgraded,this can be addressed by making use of the kernel.sol's Module::INIT function which is called during the updgrade of the module.

Note: If you don't want to introduce a new policy then you can just implement the changes by ggnoring any lines of code related to the new Emergency contract.

Make the Following changes to mitigate the issue:

  1. We Create a new Policy called Emergency.sol this works same as Laundrette.sol but with only
    required functions i.e since deposit is not required during emergency it is removed altogether

In src/policies/ add a new Policy called Emergency.sol with the code below :

Emergency.sol's Code
// SPDX-License-Identifier: MIT
​
pragma solidity 0.8.24;
​
import { Kernel, Policy, Permissions, Keycode, Role, Actions } from "src/Kernel.sol";
import { toKeycode } from "src/utils/KernelUtils.sol";
import { MoneyVault } from "src/modules/MoneyVault.sol";
​
contract Emergency is Policy {
/////////////////////////////////////////////////////////////////////////////////
// Kernel Policy Configuration //
/////////////////////////////////////////////////////////////////////////////////
​
MoneyVault private moneyVault;
​
constructor(Kernel kernel_) Policy(kernel_) { }
​
function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](1);
​
dependencies[0] = toKeycode("MONEY");
moneyVault = MoneyVault(getModuleAddress(toKeycode("MONEY")));
}
​
function requestPermissions() external view override onlyKernel returns (Permissions[] memory requests) {
requests = new Permissions[](1);
​
// requests[0] = Permissions(toKeycode("MONEY"), moneyVault.depositUSDC.selector);
requests[0] = Permissions(toKeycode("MONEY"), moneyVault.withdrawUSDC.selector);
}
​
/////////////////////////////////////////////////////////////////////////////////
// Policy Functions //
/////////////////////////////////////////////////////////////////////////////////
​
modifier isAuthorizedOrRevert(address account) {
// Only the account or the godfather
if (!(account == msg.sender || kernel.executor() == msg.sender)) {
revert("Laundrette: you are not authorized to call this function");
}
_;
}
​
modifier isGodFather() {
require(kernel.executor() == msg.sender, "Laundrette: you are not the godfather");
_;
}
​
function withdrawMoney(address account, address to, uint256 amount) external onlyRole("gangmember") {
moneyVault.withdrawUSDC(account, to, amount);
}
}
​
​

  1. Brief Explanation of Changes added below:

We need MoneyShelf's address to be transferred to MoneyVault so that we can migrate the state of MoneyShelf to MoneyVault hence we pass it as an argument to the MoneyVaults constructor.

We change kernel::admin back to godFather and grant the role of moneyshelf to MoneyVault.sol this step if not done then money transfers cannot be done i.e godFather cannot withdraw from moneyVault

EmergencyTransfer is the new function which we will add in further steps in MoneyShelf.sol to take care of state changes i.e funds stored in it

We activate a new Policy called i.e Emergency.sol

The EmergencyMigration::migrate now returns MoneyVault and Emergency types

EmergencyMigration.s.sol needs to be modified in this way

Changes in EmergencyMigration.s.sol
​
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import { Script, console2 } from "lib/forge-std/src/Script.sol";
import { IERC20 } from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
​
- import { Kernel, Actions } from "src/Kernel.sol";
+ import { Kernel, Actions, Role } from "src/Kernel.sol";
​
import { CrimeMoney } from "src/CrimeMoney.sol";
​
import { MoneyVault } from "src/modules/MoneyVault.sol";
​
+ import { Laundrette } from "src/policies/Laundrette.sol";
+ import { Emergency } from "src/policies/Emergency.sol";
+ 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));
+ Laundrette laundrette = Laundrette(address(0));
+ MoneyShelf moneyshelf = MoneyShelf(address(0));
​
- migrate(kernel, usdc, crimeMoney);
+ migrate(kernel, usdc, crimeMoney, laundrette, 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,Laundrette laundrette,MoneyShelf moneyShelf) public returns (MoneyVault, Emergency)
{
vm.startBroadcast(kernel.executor());
- MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney);
+ MoneyVault moneyVault = new MoneyVault(kernel, usdc, crimeMoney, moneyShelf);
kernel.executeAction(Actions.UpgradeModule, address(moneyVault));
​
+ Emergency emergency = new Emergency(kernel);
+ kernel.executeAction(Actions.ChangeAdmin, kernel.executor());
​
+ kernel.grantRole(Role.wrap("moneyshelf"), address(moneyVault));
+ moneyShelf.EmergencyTransfer(address(moneyVault));
​
+ kernel.executeAction(Actions.ActivatePolicy, address(emergency));
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;
+ return (moneyVault, emergency);
}
}
​

  1. This is the new EmergencyTransfer function in MoneyShelf.sol which can be called only by godFather and transfers all the money stored in MoneyShelf to address specified(MoneyVault) by godFather

Add the following function in MoneyShelf.sol

function EmergencyTransfer(address account) external {
require(msg.sender == kernel.executor(), "MoneyShelf: only GodFather can do this");
usdc.transfer(account, usdc.balanceOf(address(this)));
}

  1. In Shelf.sol we add a few Variables to track the accounts added to the mapping bank and migrate them to MoneyVault when an emergency situation happens

(Note that variables have been declared public for ease of use here, they can be made private which requires adding getter functions to read thir state )

Make the following changes in Shelf.sol (required for state transfer from MoneyShelf to MoneyVault)

Changes in Shelf.sol
​
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
​
import { Module, Kernel } from "src/Kernel.sol";
​
​
abstract contract Shelf is Module {
mapping(address => uint256) public bank;
​
+ mapping(address => bool) public record;
+ address[] public accounts;
+ uint256 public length;
​
// IERC20 private immutable token;
​
constructor(Kernel kernel_) Module(kernel_) { }
​
function deposit(address account, uint256 amount) public permissioned {
bank[account] += amount;
​
+ if (!record[account]) {
+ accounts.push(account);
+ length++;
+ record[account] = true;
}
}
​
....
...
..
.
}
​

  1. Now when even the module MoneyShelf is upgraded to MoneyVault the INIT function is triggered which transfers the state of the MoneyShelf to MoneyVault , we transfer the actual funds in EmergencyMigration::migrate by calling the EmergencyTransfer function of MoneyShelf with the address of moneyVault

Make the following changes in MoneyVault.sol

Changes in MoneyVault.sol
​
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
​
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { CrimeMoney } from "src/CrimeMoney.sol";
​
import { Kernel, Keycode } from "src/Kernel.sol";
​
import { Shelf } from "src/modules/Shelf.sol";
​
+ import { MoneyShelf } from "src/modules/MoneyShelf.sol";
​
// Emergency money vault to store USDC and
​
contract MoneyVault is Shelf {
IERC20 private usdc;
CrimeMoney private crimeMoney;
​
+ Shelf private oldShelf;
​
- constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney) Shelf(kernel_) {
+ constructor(Kernel kernel_, IERC20 _usdc, CrimeMoney _crimeMoney, MoneyShelf _moneyShelf) Shelf(kernel_) {
usdc = _usdc;
crimeMoney = _crimeMoney;
+ oldShelf = Shelf(address(_moneyShelf));
}
​
​
+ function INIT() external override onlyKernel {
+ uint256 len = oldShelf.length();
+ for (uint256 i; i < len; i++) {
+ bank[oldShelf.accounts(i)] = oldShelf.bank(oldShelf.accounts(i));
+ }
+ }
​
function KEYCODE() public pure override returns (Keycode) {
return Keycode.wrap("MONEY");
}
......
....
..
.
}
}

  1. Now finally add the following test to EmergencyMigration.t.sol and run it

(if any errors show up it is because we added new args to migrate function ,make changes accordingly in other tests or comment them out)

Test to check EmergencyMigration works properly
​
function test_MigrationWorksAndGodFatherCanWithdrawUSDC() public {
vm.prank(godFather);
usdc.transfer(address(this), 100e6);
​
//god father balance after transferring 100e6
uint256 godFatherBal1 = usdc.balanceOf(address(godFather));
​
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(address(this), address(this), 100e6);
assertEq(usdc.balanceOf(address(this)), 0);
assertEq(usdc.balanceOf(address(moneyShelf)), 100e6);
assertEq(crimeMoney.balanceOf(address(this)), 100e6);
​
moneyShelf.getAccountAmount(address(this));
​
joinGang(address(this));
​
// migration takes place here
// notice that migrate() returns Emergency type
EmergencyMigration migration = new EmergencyMigration();
(MoneyVault moneyVault, Emergency emergency) =
migration.migrate(kernel, usdc, crimeMoney, laundrette, moneyShelf);
// we withdraw from MoneyVault using new emergency policy
emergency.withdrawMoney(address(this), godFather, 100e6);
//Final assertion
assertEq(usdc.balanceOf(address(godFather)), godFatherBal1 + 100e6);
}
​
Updates

Lead Judging Commences

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

Emergency migration leave the USDC

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.