From 54f328458b21938c95dc965cb79b606258a0f9e9 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Tue, 27 Feb 2024 15:29:11 -0300 Subject: [PATCH] [SC-324] Deny can be triggered by auth (#8) * deny can be triggered by auth * add spell and integration tests * add new spell to readme; give a good description of what the setup should be * test all functions are de-authed --- README.md | 9 ++ src/SparkLendFreezerMom.sol | 2 +- ...mergencySpell_SparkLend_RemoveMultisig.sol | 25 ++++ test/IntegrationTests.t.sol | 108 +++++++++++++----- test/SparkLendFreezerMom.t.sol | 38 +++++- 5 files changed, 153 insertions(+), 29 deletions(-) create mode 100644 src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol diff --git a/README.md b/README.md index e0d11ce..60232b2 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,15 @@ A spell that can be set as the `hat` in the Chief to pauses all markets in Spark ### `src/spells/EmergencySpell_SparkLend_PauseSingleAsset.sol` A spell that can be set as the `hat` in the Chief to pause a specific market in SparkLend by calling `pauseMarket(reserve, true)` in `SparkLendFreezerMom`. A separate spell is needed for each market, with the reserve being declared in the constructor. +### `src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol` +A spell that can be set as the `hat` in the Chief to remove a ward by calling `deny(multisig)` in `SparkLendFreezerMom`. + +## Expected Configuration + +It is important that the code be matched to a very specific setup to ensure all assumptions are valid. In particular, it is expected that the owner will be the Spark SubDAO Proxy (0x3300f198988e4C9C63F75dF86De36421f06af8c4), the authority will be the MCD Chief (0x0a3f6849f78076aefaDf113F5BED87720274dDC0) and there will at most 1 ward that is a SAFE multisig. + +This configuration is important because `deny` is set to only required `auth` parameters, so there are three ways to remove the multisig (multisig itself, remove multisig spell without GSM delay, and finally the Spark SubDAO Proxy). This ensures that MKR holders can override a decision made by the multisig (and de-auth it) without a GSM delay. Furthermore the SubDAO Proxy behind a delay can override any decision of a non-delay vote and the multisig (and de-auth both of them). + ## Execution Flow Diagrams The below diagrams outlines the execution flow of freezing a single market in SparkLend for both token-based governance in MakerDAO and the emergency multisig, which will be added as a `ward` in the FreezerMom. diff --git a/src/SparkLendFreezerMom.sol b/src/SparkLendFreezerMom.sol index 12e89a3..c98a22e 100644 --- a/src/SparkLendFreezerMom.sol +++ b/src/SparkLendFreezerMom.sol @@ -66,7 +66,7 @@ contract SparkLendFreezerMom is ISparkLendFreezerMom { emit Rely(usr); } - function deny(address usr) external override onlyOwner { + function deny(address usr) external override auth { wards[usr] = 0; emit Deny(usr); } diff --git a/src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol b/src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol new file mode 100644 index 0000000..87845f7 --- /dev/null +++ b/src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +pragma solidity ^0.8.13; + +import { IExecuteOnceSpell } from "src/interfaces/IExecuteOnceSpell.sol"; +import { ISparkLendFreezerMom } from "src/interfaces/ISparkLendFreezerMom.sol"; + +contract EmergencySpell_SparkLend_RemoveMultisig is IExecuteOnceSpell { + + address public immutable sparkLendFreezerMom; + address public immutable multisig; + + bool public override executed; + + constructor(address sparkLendFreezerMom_, address multisig_) { + sparkLendFreezerMom = sparkLendFreezerMom_; + multisig = multisig_; + } + + function execute() external override { + require(!executed, "RemoveMultisigSpell/already-executed"); + executed = true; + ISparkLendFreezerMom(sparkLendFreezerMom).deny(multisig); + } + +} diff --git a/test/IntegrationTests.t.sol b/test/IntegrationTests.t.sol index b89eb97..1168e73 100644 --- a/test/IntegrationTests.t.sol +++ b/test/IntegrationTests.t.sol @@ -21,6 +21,9 @@ import { EmergencySpell_SparkLend_PauseSingleAsset as PauseSingleAssetSpell } import { EmergencySpell_SparkLend_PauseAllAssets as PauseAllAssetsSpell } from "src/spells/EmergencySpell_SparkLend_PauseAllAssets.sol"; +import { EmergencySpell_SparkLend_RemoveMultisig as RemoveMultisigSpell } + from "src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol"; + import { IACLManager } from "lib/aave-v3-core/contracts/interfaces/IACLManager.sol"; import { IPoolConfigurator } from "lib/aave-v3-core/contracts/interfaces/IPoolConfigurator.sol"; import { IPoolDataProvider } from "lib/aave-v3-core/contracts/interfaces/IPoolDataProvider.sol"; @@ -229,18 +232,8 @@ contract IntegrationTestsBase is Test { abstract contract ExecuteOnceSpellTests is IntegrationTestsBase { IExecuteOnceSpell spell; - bool isPauseSpell; string contractName; - function setUp() public virtual override { - super.setUp(); - - vm.startPrank(SPARK_PROXY); - aclManager.addEmergencyAdmin(address(freezerMom)); - aclManager.addRiskAdmin(address(freezerMom)); - vm.stopPrank(); - } - function _vote() internal { _vote(address(spell)); } @@ -281,12 +274,7 @@ abstract contract ExecuteOnceSpellTests is IntegrationTestsBase { spell.execute(); } - function test_cannotCallWithoutRoleSetup() external { - vm.startPrank(SPARK_PROXY); - aclManager.removeEmergencyAdmin(address(freezerMom)); - aclManager.removeRiskAdmin(address(freezerMom)); - vm.stopPrank(); - + function test_cannotCallTwice() external { _vote(); assertTrue(authority.hat() == address(spell)); @@ -298,12 +286,34 @@ abstract contract ExecuteOnceSpellTests is IntegrationTestsBase { ) ); - if (isPauseSpell) vm.expectRevert(bytes("3")); // CALLER_NOT_POOL_OR_EMERGENCY_ADMIN - else vm.expectRevert(bytes("4")); // CALLER_NOT_RISK_OR_POOL_ADMIN + vm.startPrank(randomUser); // Demonstrate no ACL in spell + spell.execute(); + + vm.expectRevert(bytes(string.concat(contractName, "/already-executed"))); spell.execute(); } - function test_cannotCallTwice() external { +} + +abstract contract FreezePauseSpellTests is ExecuteOnceSpellTests { + + bool isPauseSpell; + + function setUp() public virtual override { + super.setUp(); + + vm.startPrank(SPARK_PROXY); + aclManager.addEmergencyAdmin(address(freezerMom)); + aclManager.addRiskAdmin(address(freezerMom)); + vm.stopPrank(); + } + + function test_cannotCallWithoutRoleSetup() external { + vm.startPrank(SPARK_PROXY); + aclManager.removeEmergencyAdmin(address(freezerMom)); + aclManager.removeRiskAdmin(address(freezerMom)); + vm.stopPrank(); + _vote(); assertTrue(authority.hat() == address(spell)); @@ -315,16 +325,14 @@ abstract contract ExecuteOnceSpellTests is IntegrationTestsBase { ) ); - vm.startPrank(randomUser); // Demonstrate no ACL in spell - spell.execute(); - - vm.expectRevert(bytes(string.concat(contractName, "/already-executed"))); + if (isPauseSpell) vm.expectRevert(bytes("3")); // CALLER_NOT_POOL_OR_EMERGENCY_ADMIN + else vm.expectRevert(bytes("4")); // CALLER_NOT_RISK_OR_POOL_ADMIN spell.execute(); } } -contract FreezeSingleAssetSpellTest is ExecuteOnceSpellTests { +contract FreezeSingleAssetSpellTest is FreezePauseSpellTests { using SafeERC20 for IERC20; @@ -415,7 +423,7 @@ contract FreezeSingleAssetSpellTest is ExecuteOnceSpellTests { } -contract FreezeAllAssetsSpellTest is ExecuteOnceSpellTests { +contract FreezeAllAssetsSpellTest is FreezePauseSpellTests { using SafeERC20 for IERC20; @@ -513,7 +521,7 @@ contract FreezeAllAssetsSpellTest is ExecuteOnceSpellTests { } -contract PauseSingleAssetSpellTest is ExecuteOnceSpellTests { +contract PauseSingleAssetSpellTest is FreezePauseSpellTests { using SafeERC20 for IERC20; @@ -604,7 +612,7 @@ contract PauseSingleAssetSpellTest is ExecuteOnceSpellTests { } -contract PauseAllAssetsSpellTest is ExecuteOnceSpellTests { +contract PauseAllAssetsSpellTest is FreezePauseSpellTests { using SafeERC20 for IERC20; @@ -791,3 +799,49 @@ contract MultisigTest is IntegrationTestsBase { } } + +contract RemoveMultisigSpellTest is ExecuteOnceSpellTests { + + function setUp() public override { + super.setUp(); + + // For the revert testing + spell = new RemoveMultisigSpell(address(freezerMom), multisig); + contractName = "RemoveMultisigSpell"; + + vm.prank(SPARK_PROXY); + aclManager.addRiskAdmin(address(freezerMom)); + } + + function test_removeMultisigSpell_multisig() external { + assertEq(freezerMom.wards(multisig), 1); + + // Verify multisig can freeze and unfreeze markets + vm.startPrank(multisig); + assertEq(_isFrozen(WETH), false); + freezerMom.freezeMarket(WETH, true); + assertEq(_isFrozen(WETH), true); + freezerMom.freezeMarket(WETH, false); + assertEq(_isFrozen(WETH), false); + vm.stopPrank(); + + _vote(); + vm.prank(randomUser); // Demonstrate no ACL in spell + spell.execute(); + + assertEq(freezerMom.wards(multisig), 0); + + // Verify multisig can no longer freeze and unfreeze markets + vm.startPrank(multisig); + vm.expectRevert("SparkLendFreezerMom/not-authorized"); + freezerMom.freezeMarket(WETH, true); + vm.expectRevert("SparkLendFreezerMom/not-authorized"); + freezerMom.pauseMarket(WETH, true); + vm.expectRevert("SparkLendFreezerMom/not-authorized"); + freezerMom.freezeAllMarkets(true); + vm.expectRevert("SparkLendFreezerMom/not-authorized"); + freezerMom.pauseAllMarkets(true); + vm.stopPrank(); + } + +} diff --git a/test/SparkLendFreezerMom.t.sol b/test/SparkLendFreezerMom.t.sol index 7d1f6f7..1f42b41 100644 --- a/test/SparkLendFreezerMom.t.sol +++ b/test/SparkLendFreezerMom.t.sol @@ -105,7 +105,7 @@ contract RelyTests is SparkLendFreezerMomUnitTestBase { contract DenyTests is SparkLendFreezerMomUnitTestBase { function test_deny_noAuth() public { - vm.expectRevert("SparkLendFreezerMom/only-owner"); + vm.expectRevert("SparkLendFreezerMom/not-authorized"); freezer.deny(makeAddr("authedContract")); } @@ -123,6 +123,42 @@ contract DenyTests is SparkLendFreezerMomUnitTestBase { assertEq(freezer.wards(authedContract), 0); } + function test_deny_ward() public { + address authedContract = makeAddr("authedContract"); + + vm.prank(owner); + freezer.rely(authedContract); + + assertEq(freezer.wards(authedContract), 1); + + vm.prank(authedContract); + freezer.deny(authedContract); + + assertEq(freezer.wards(authedContract), 0); + } + + function test_deny_authorized_spell() public { + address authedContract = makeAddr("authedContract"); + address caller = makeAddr("caller"); + + authority.__setCanCall( + caller, + address(freezer), + freezer.deny.selector, + true + ); + + vm.prank(owner); + freezer.rely(authedContract); + + assertEq(freezer.wards(authedContract), 1); + + vm.prank(caller); + freezer.deny(authedContract); + + assertEq(freezer.wards(authedContract), 0); + } + } contract FreezeAllMarketsTests is SparkLendFreezerMomUnitTestBase {