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

Omitted policy reconfig during module upgrade due to misassigned `dependencies` array in `Laundrette::configureDependencies`

paprikrumplikas

Summary

Laundrette::configureDependencies contains a bug (misassigned dependencies array) that results in the omission of Laundrette's policy reconfiguration when the module associated with the MONEY keycode is updated. Laundrette will keep working on the old module instead of the new one.

Vulnerability Details

Laundrette::configureDependencies is a function called by the Kernel when the Laundrette policy is activated or reconfigured. This function is supposed to ensure that the policy's dependencies on specific modules are properly set up.

However, these dependencies are not set correctly. dependencies in Laundrette is supposed to be a 2-element array containing the 2 different keycodes of the 2 modules the policy depends on, but in practice the first element (dependencies[0]) is overwritten by the WEAPN KeyCode and the second one is left empty:

function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](2);
​
@> dependencies[0] = toKeycode("MONEY");
moneyShelf = MoneyShelf(getModuleAddress(toKeycode("MONEY")));
​
@> dependencies[0] = toKeycode("WEAPN");
weaponShelf = WeaponShelf(getModuleAddress(toKeycode("WEAPN")));
}

Impact

In short,

  • the policy (laundrette) will not be reconfigured when moneyShelf is upgraded to moneyVault, and consequently

  • laundrette's internal references to moneyShelf be not be updated, it will keep using the moneyShelf module that was supposed to be upgraded.

Notably, the impact of all this can be contained if, after the migration, the godfather (as kernel's executor) deactivates and then reactivates the policy:

... module update
​
kernel.executeAction(Actions.DeactivatePolicy, address(laundrette));
kernel.executeAction(Actions.ActivatePolicy, address(laundrette));
Longer explanation

To determine the impact, we need to consider where and how the incorrectly set up dependencies array is used. It is passed down as a return value to Kernel which then uses it to define 2 mappings:

function _activatePolicy(Policy policy_) internal {
if (policy_.isActive()) revert Kernel_PolicyAlreadyApproved(address(policy_));
​
// Grant permissions for policy to access restricted module functions
Permissions[] memory requests = policy_.requestPermissions();
_setPolicyPermissions(policy_, requests, true);
​
// Add policy to list of active policies
activePolicies.push(policy_);
getPolicyIndex[policy_] = activePolicies.length - 1;
​
// Record module dependencies
Keycode[] memory dependencies = policy_.configureDependencies();
uint256 depLength = dependencies.length;
​
for (uint256 i; i < depLength;) {
Keycode keycode = dependencies[i];
​
@> moduleDependents[keycode].push(policy_);
@> getDependentIndex[keycode][policy_] = moduleDependents[keycode].length - 1;
​
unchecked {
++i;
}
}

These mappings will obviously be incorrect (incomplete), but the real impact unfolds when Kernel tries to use these mappings:

Impact of the incorrect `moduleDependents`

  • Impact - where & when:
    apart from Kernel::_activatePolicy (which uses the incorrect dependencies array from Laundrette to set up the mappings), it is only used by Kernel::_reconfigurePolicies which, in turn, is only relied on by Kernel::_upgradeModule.

  • Impact - what:
    Considering the code of Kernel::_reconfigurePolicies below, it can be seen that if the MONEY keycode (that has never been added to moduleDependents, i.e. as a dependency to the policy) is provided as an input parameter (happens when the module associated with the MONEY keycode is being upgraded), then

-- depLength will be 0 and configureDependencies will never be called on the policy, so

-- policy reconfiguration never happens,

-- Laundrettes internal references to the dependencies are not updated and, hence,

-- it will keep working with the old moneyShelf module.

function _reconfigurePolicies(Keycode keycode_) internal {
Policy[] memory dependents = moduleDependents[keycode_];
uint256 depLength = dependents.length;
​
for (uint256 i; i < depLength;) {
dependents[i].configureDependencies();
​
unchecked {
++i;
}
}
}

Notably, the impact of all this can be contained if, after the migration, the godfather (as kernel's executor) deactivates and then reactivates the policy.

Impact of the incorrect `getDependentIndex` array

  • Impact - where & when: getDependentIndex is only used by Kernel::_pruneFromDependents which, in turn, can only be called by Kernel::_deactivatePolicy.

  • Impact - what: none.

Place the following test to Laundrette.t.sol to demonstrate that

  • the policy is not reconfigured after an update from module moneyShelf to moneyVault,

  • the policy keeps working on moneyShelf and not on moneyVault after the upgrade,

  • the issue can be mitigated if the godfather deactivates the policy than activates it again.

Proof of Code
​
// @notice import: import { Policy } from "src/Kernel.sol";
// @notice add: import { EmergencyMigration } from "script/EmergencyMigration.s.sol";
// @notice add: import { MoneyVault } from "src/modules/MoneyVault.sol";
// @notice add: import { Keycode } from "src/Kernel.sol";
function testMissedPolicyReconfig() public {
/////////////////////////////////////////////////////////
/// SETUP - GODFATHER DEPOSITS TO MONEYSHELF ///////////
/////////////////////////////////////////////////////////
​
uint256 depositAmount = 1e12;
​
vm.prank(kernel.admin());
kernel.grantRole(Role.wrap("gangmember"), godFather);
​
vm.startPrank(godFather);
usdc.approve(address(moneyShelf), depositAmount);
laundrette.depositTheCrimeMoneyInATM(godFather, godFather, depositAmount);
vm.stopPrank();
​
/////////////////////////////////////////////////////////
/// CHECK THAT BOTH KEXCODES ARE RECOGNIZED BY KERNEL ///
/////////////////////////////////////////////////////////
​
Keycode keycode_0 = kernel.allKeycodes(0);
string memory keycodeStr_0 = bytes5ToString(Keycode.unwrap(keycode_0));
assertEq(keycodeStr_0, "WEAPN"); // because WEAPONSHELF was installed first
​
Keycode keycode_1 = kernel.allKeycodes(1);
string memory keycodeStr_1 = bytes5ToString(Keycode.unwrap(keycode_1));
assertEq(keycodeStr_1, "MONEY"); // because MONEYSHELF was installed second
​
/////////////////////////////////////////////////////////
/// CHECK WHICH POLICY DEPEND ON KEYCODE "WEAPN" ////////
/////////////////////////////////////////////////////////
​
Policy policy = kernel.moduleDependents(keycode_0, 0);
assertEq(address(policy), address(laundrette));
​
/////////////////////////////////////////////////////////
/// CHECK WHICH POLICY DEPEND ON KEYCODE "WEAPN" ////////
/////////////////////////////////////////////////////////
​
vm.expectRevert(); // laundrette policy appears to not depend on the MONEY keycode
policy = kernel.moduleDependents(keycode_1, 0);
​
/////////////////////////////////////////////////////////
///////////////// MODULE UPGRADE ////////////////////////
/////////////////////////////////////////////////////////
​
// initially, Kernel recognizes moneyShelf as the module associated with keycode MONEY - OK
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyShelf));
​
EmergencyMigration migration;
migration = new EmergencyMigration();
MoneyVault moneyVault = migration.migrate(kernel, usdc, crimeMoney); // migration
​
// now Kernel recognizes moneyShelf as the module associated with keycode MONEY - OK
assertEq(address(kernel.getModuleForKeycode(Keycode.wrap("MONEY"))), address(moneyVault));
​
/////////////////////////////////////////////////////////
///////// PROOF THAT POLICY HAS NOT BEEN UPDATED ////////
/////////////////////////////////////////////////////////
​
uint256 withdrawAmount = depositAmount / 2;
uint256 initialMoneyShelfBalance = usdc.balanceOf(address(moneyShelf));
uint256 endingMoneyShelfBalance;
​
vm.prank(godFather);
laundrette.withdrawMoney(godFather, godFather, withdrawAmount);
​
endingMoneyShelfBalance = usdc.balanceOf(address(moneyShelf));
​
// initial and ending moneyShelf balances are not the same
// --> laundrette.withdrawMoney worked on moneyShelf, not moneyVault
assertLt(endingMoneyShelfBalance, initialMoneyShelfBalance);
​
/////////////////////////////////////////////////////////
///////// THE ISSUE CAN BE MITIGATED ////////////////////
/////////////////////////////////////////////////////////
​
// setup a non-zero balance for moneyVault
vm.startPrank(godFather);
kernel.executeAction(Actions.DeactivatePolicy, address(laundrette)); // mitigation step1
kernel.executeAction(Actions.ActivatePolicy, address(laundrette)); // mitigation step2
​
// expect laundrette.withdrawMoney to work on moneyVault, but it has no balance
// so trx will revert with artihmetic under/overflow
vm.expectRevert();
laundrette.withdrawMoney(godFather, godFather, withdrawAmount);
vm.stopPrank();
​
// the remaining balance of moneyShelf did not change -> 2nd laundrette.withdrawMoney did not work on moneyShelf
assertEq(usdc.balanceOf(address(moneyShelf)), depositAmount / 2);
}

Tools Used

Manual review, Foundry.

Recommendations

Correct Laundrette::configureDependencies as follows:

function configureDependencies() external override onlyKernel returns (Keycode[] memory dependencies) {
dependencies = new Keycode[](2);
​
dependencies[0] = toKeycode("MONEY");
moneyShelf = MoneyShelf(getModuleAddress(toKeycode("MONEY")));
​
- dependencies[0] = toKeycode("WEAPN");
+ dependencies[0] = toKeycode("WEAPN");
weaponShelf = WeaponShelf(getModuleAddress(toKeycode("WEAPN")));
}
Updates

Lead Judging Commences

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

Laundrette incorrect dependencies

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

Laundrette incorrect dependencies

Support

FAQs

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