Skip to content

Commit

Permalink
[SC-324] Deny can be triggered by auth (#8)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@hexonaut
hexonaut authored Feb 27, 2024
1 parent 79bb947 commit 54f3284
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 29 deletions.
9 changes: 9 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/SparkLendFreezerMom.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
25 changes: 25 additions & 0 deletions src/spells/EmergencySpell_SparkLend_RemoveMultisig.sol
View file
Original file line number Diff line number Diff line change
@@ -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);
}

}
108 changes: 81 additions & 27 deletions test/IntegrationTests.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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;

Expand Down Expand Up @@ -415,7 +423,7 @@ contract FreezeSingleAssetSpellTest is ExecuteOnceSpellTests {

}

contract FreezeAllAssetsSpellTest is ExecuteOnceSpellTests {
contract FreezeAllAssetsSpellTest is FreezePauseSpellTests {

using SafeERC20 for IERC20;

Expand Down Expand Up @@ -513,7 +521,7 @@ contract FreezeAllAssetsSpellTest is ExecuteOnceSpellTests {

}

contract PauseSingleAssetSpellTest is ExecuteOnceSpellTests {
contract PauseSingleAssetSpellTest is FreezePauseSpellTests {

using SafeERC20 for IERC20;

Expand Down Expand Up @@ -604,7 +612,7 @@ contract PauseSingleAssetSpellTest is ExecuteOnceSpellTests {

}

contract PauseAllAssetsSpellTest is ExecuteOnceSpellTests {
contract PauseAllAssetsSpellTest is FreezePauseSpellTests {

using SafeERC20 for IERC20;

Expand Down Expand Up @@ -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();
}

}
38 changes: 37 additions & 1 deletion test/SparkLendFreezerMom.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
}

Expand All @@ -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 {
Expand Down

0 comments on commit 54f3284

Please sign in to comment.