Skip to content

N
nebula
Commit 4424552c authored by Maurelian's avatar Maurelian Committed by GitHub
Browse files
Options

ctb: Add DeputyGuardianModule (#9982) 

* ctb: Allow the Liveness Module's threshold to be customized

* ctb: Hardcode threshold percentage as immutable in livenessmodule

* ctb: test threshold math differentially

* ctb: Threshold tests with hardcoded values and boundary fuzz tests
parent 02c1193e
Hide whitespace changes
Inline Side-by-side
Showing with 189 additions and 66 deletions
packages/contracts-bedrock/semver-lock.json
View file @ 4424552c
......@@ -92,8 +92,8 @@
"sourceCodeHash": "0x9633cea9b66077e222f470439fe3e9a31f3e33b4f7a5618374c44310fd234b24"
},
"src/Safe/LivenessModule.sol": {
"initCodeHash": "0xcd8b76f70634330e242d4ff497bba1f8e0aaa11d7aecf9845090029c43027a7f",
"sourceCodeHash": "0x3b358b51fce4f1516191104d2e1f782c8ec3edecee63c502cc301671aa632b93"
"initCodeHash": "0xa8b233f0f26f8a73b997b12ba06d64cefa8ee98d523f68cd63320e9787468fae",
"sourceCodeHash": "0x73aa5934e56ba2a45f368806c5db1d442bf5713d51b2184749f4638eaceb832e"
},
"src/cannon/MIPS.sol": {
"initCodeHash": "0xaf2ac814f64ccf12e9c6738db7cef865f51f9e39f39105adef9fba11465f6ee1",
......
packages/contracts-bedrock/snapshots/abi/LivenessModule.json
View file @ 4424552c
......@@ -21,6 +21,11 @@
"name": "_minOwners",
"type": "uint256"
},
{
"internalType": "uint256",
"name": "_thresholdPercentage",
"type": "uint256"
},
{
"internalType": "address",
"name": "_fallbackOwner",
......@@ -70,7 +75,7 @@
"type": "uint256"
}
],
"name": "get75PercentThreshold",
"name": "getRequiredThreshold",
"outputs": [
{
"internalType": "uint256",
......@@ -78,7 +83,7 @@
"type": "uint256"
}
],
"stateMutability": "pure",
"stateMutability": "view",
"type": "function"
},
{
......@@ -151,6 +156,19 @@
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "thresholdPercentage",
"outputs": [
{
"internalType": "uint256",
"name": "thresholdPercentage_",
"type": "uint256"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "version",
......
packages/contracts-bedrock/src/Safe/LivenessModule.sol
View file @ 4424552c
......@@ -35,6 +35,9 @@ contract LivenessModule is ISemver {
/// This can be updated by replacing with a new module.
uint256 internal immutable MIN_OWNERS;
/// @notice The percentage used to calculate the threshold for the Safe.
uint256 internal immutable THRESHOLD_PERCENTAGE;
/// @notice The fallback owner of the Safe
/// This can be updated by replacing with a new module.
address internal immutable FALLBACK_OWNER;
......@@ -44,8 +47,8 @@ contract LivenessModule is ISemver {
uint256 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;
/// @notice Semantic version.
/// @custom:semver 1.0.0
string public constant version = "1.0.0";
/// @custom:semver 1.1.0
string public constant version = "1.1.0";
// Constructor to initialize the Safe and baseModule instances
constructor(
......@@ -53,25 +56,29 @@ contract LivenessModule is ISemver {
LivenessGuard _livenessGuard,
uint256 _livenessInterval,
uint256 _minOwners,
uint256 _thresholdPercentage,
address _fallbackOwner
) {
SAFE = _safe;
LIVENESS_GUARD = _livenessGuard;
LIVENESS_INTERVAL = _livenessInterval;
THRESHOLD_PERCENTAGE = _thresholdPercentage;
FALLBACK_OWNER = _fallbackOwner;
MIN_OWNERS = _minOwners;
address[] memory owners = _safe.getOwners();
require(_minOwners <= owners.length, "LivenessModule: minOwners must be less than the number of owners");
require(
_safe.getThreshold() >= get75PercentThreshold(owners.length),
"LivenessModule: Safe must have a threshold of at least 75% of the number of owners"
_safe.getThreshold() >= getRequiredThreshold(owners.length),
"LivenessModule: Insufficient threshold for the number of owners"
);
require(_thresholdPercentage > 0, "LivenessModule: thresholdPercentage must be greater than 0");
require(_thresholdPercentage <= 100, "LivenessModule: thresholdPercentage must be less than or equal to 100");
}
/// @notice For a given number of owners, return the lowest threshold which is greater than 75.
/// @notice For a given number of owners, return the lowest threshold which is greater than the required percentage.
/// Note: this function returns 1 for numOwners == 1.
function get75PercentThreshold(uint256 _numOwners) public pure returns (uint256 threshold_) {
threshold_ = (_numOwners * 75 + 99) / 100;
function getRequiredThreshold(uint256 _numOwners) public view returns (uint256 threshold_) {
threshold_ = (_numOwners * THRESHOLD_PERCENTAGE + 99) / 100;
}
/// @notice Getter function for the Safe contract instance
......@@ -98,7 +105,13 @@ contract LivenessModule is ISemver {
minOwners_ = MIN_OWNERS;
}
/// @notice Getter function for the fallback owner
/// @notice Getter function for the required threshold percentage
/// @return thresholdPercentage_ The minimum number of owners
function thresholdPercentage() public view returns (uint256 thresholdPercentage_) {
thresholdPercentage_ = THRESHOLD_PERCENTAGE;
}
/// @notice Getter function for the fallback
/// @return fallbackOwner_ The fallback owner of the Safe
function fallbackOwner() public view returns (address fallbackOwner_) {
fallbackOwner_ = FALLBACK_OWNER;
......@@ -161,7 +174,7 @@ contract LivenessModule is ISemver {
/// @param _newOwnersCount New number of owners after removal.
function _removeOwner(address _prevOwner, address _ownerToRemove, uint256 _newOwnersCount) internal {
if (_newOwnersCount > 0) {
uint256 newThreshold = get75PercentThreshold(_newOwnersCount);
uint256 newThreshold = getRequiredThreshold(_newOwnersCount);
// Remove the owner and update the threshold
_removeOwnerSafeCall({ _prevOwner: _prevOwner, _owner: _ownerToRemove, _threshold: newThreshold });
} else {
......@@ -223,11 +236,11 @@ contract LivenessModule is ISemver {
// Check that"LivenessModule: must remove all owners and transfer to fallback owner if numOwners < minOwners"
// the threshold is correct. This check is also correct when there is a single
// owner, because get75PercentThreshold(1) returns 1.
// owner, because getRequiredThreshold(1) returns 1.
uint256 threshold = SAFE.getThreshold();
require(
threshold == get75PercentThreshold(numOwners),
"LivenessModule: Safe must have a threshold of 75% of the number of owners"
threshold == getRequiredThreshold(numOwners),
"LivenessModule: Insufficient threshold for the number of owners"
);
// Check that the guard has not been changed
......
packages/contracts-bedrock/test/Safe/LivenessModule.t.sol
View file @ 4424552c
......@@ -20,9 +20,10 @@ contract LivenessModule_TestInit is Test, SafeTestTools {
event RemovedOwner(address indexed owner);
event OwnershipTransferredToFallback();
uint256 initTime = 10;
uint256 livenessInterval = 30 days;
uint256 minOwners = 6;
uint256 constant INIT_TIME = 10;
uint256 constant LIVENESS_INTERVAL = 30 days;
uint256 constant MIN_OWNERS = 6;
uint256 constant THRESHOLD_PERCENTAGE = 75;
LivenessModule livenessModule;
LivenessGuard livenessGuard;
SafeInstance safeInstance;
......@@ -40,14 +41,35 @@ contract LivenessModule_TestInit is Test, SafeTestTools {
/// @dev Set the current time to after the liveness interval
function _warpPastLivenessInterval() internal {
vm.warp(initTime + livenessInterval + 1);
vm.warp(INIT_TIME + LIVENESS_INTERVAL + 1);
}
/// @dev Helper function to calculate the required threshold for a given number of owners and threshold percentage.
/// This does a lot of extra work to set up a dummy LivenessModule and Safe, but helps to keep the tests clean.
function _getRequiredThreshold(uint256 _numOwners, uint256 _thresholdPercentage) internal returns (uint256) {
// Mock calls made to the Safe by the LivenessModule's constructor
address[] memory dummyOwners = new address[](_numOwners);
dummyOwners[0] = address(0);
vm.mockCall(address(0), abi.encodeCall(OwnerManager.getOwners, ()), abi.encode(dummyOwners));
vm.mockCall(address(0), abi.encodeCall(OwnerManager.getThreshold, ()), abi.encode(dummyOwners.length));
// This module is only used to calculate the required threshold for the Safe. It's state will not be modified
// after it is created so the constructor arguments are not important.
LivenessModule dummyLivenessModule = new LivenessModule({
_safe: Safe(payable(address(0))),
_livenessGuard: LivenessGuard(address(0)),
_livenessInterval: 0,
_thresholdPercentage: _thresholdPercentage,
_minOwners: 0,
_fallbackOwner: address(0)
});
return dummyLivenessModule.getRequiredThreshold(_numOwners);
}
/// @dev Sets up the test environment
function setUp() public virtual {
// Set the block timestamp to the initTime, so that signatures recorded in the first block
// are non-zero.
vm.warp(initTime);
vm.warp(INIT_TIME);
// Create a Safe with 10 owners
(, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("moduleTest", 10);
......@@ -58,8 +80,9 @@ contract LivenessModule_TestInit is Test, SafeTestTools {
livenessModule = new LivenessModule({
_safe: safeInstance.safe,
_livenessGuard: livenessGuard,
_livenessInterval: livenessInterval,
_minOwners: minOwners,
_livenessInterval: LIVENESS_INTERVAL,
_thresholdPercentage: THRESHOLD_PERCENTAGE,
_minOwners: MIN_OWNERS,
_fallbackOwner: fallbackOwner
});
safeInstance.setGuard(address(livenessGuard));
......@@ -74,24 +97,26 @@ contract LivenessModule_Constructor_TestFail is LivenessModule_TestInit {
new LivenessModule({
_safe: safeInstance.safe,
_livenessGuard: livenessGuard,
_livenessInterval: livenessInterval,
_minOwners: 11,
_livenessInterval: LIVENESS_INTERVAL,
_thresholdPercentage: THRESHOLD_PERCENTAGE,
_minOwners: safeInstance.owners.length + 1,
_fallbackOwner: address(0)
});
}
/// @dev Tests that the constructor fails if the minOwners is greater than the number of owners
function test_constructor_wrongThreshold_reverts() external {
uint256 wrongThreshold = livenessModule.get75PercentThreshold(safeInstance.owners.length) - 1;
uint256 wrongThreshold = livenessModule.getRequiredThreshold(safeInstance.owners.length) - 1;
vm.mockCall(
address(safeInstance.safe), abi.encodeCall(OwnerManager.getThreshold, ()), abi.encode(wrongThreshold)
);
vm.expectRevert("LivenessModule: Safe must have a threshold of at least 75% of the number of owners");
vm.expectRevert("LivenessModule: Insufficient threshold for the number of owners");
new LivenessModule({
_safe: safeInstance.safe,
_livenessGuard: livenessGuard,
_livenessInterval: livenessInterval,
_minOwners: minOwners,
_livenessInterval: LIVENESS_INTERVAL,
_thresholdPercentage: THRESHOLD_PERCENTAGE,
_minOwners: MIN_OWNERS,
_fallbackOwner: address(0)
});
}
......@@ -104,6 +129,8 @@ contract LivenessModule_Getters_Test is LivenessModule_TestInit {
assertEq(address(livenessModule.livenessGuard()), address(livenessGuard));
assertEq(livenessModule.livenessInterval(), 30 days);
assertEq(livenessModule.minOwners(), 6);
assertEq(livenessModule.thresholdPercentage(), THRESHOLD_PERCENTAGE);
assertEq(safeInstance.safe.getThreshold(), livenessModule.getRequiredThreshold(safeInstance.owners.length));
assertEq(livenessModule.fallbackOwner(), fallbackOwner);
}
}
......@@ -126,30 +153,93 @@ contract LivenessModule_CanRemove_Test is LivenessModule_TestInit {
}
}
contract LivenessModule_Get75PercentThreshold_Test is LivenessModule_TestInit {
/// @dev check the return values of the get75PercentThreshold function against manually
contract LivenessModule_GetRequiredThreshold_Test is LivenessModule_TestInit {
/// @dev Tests if getRequiredThreshold work correctly by implementing the same logic in a different manner
function _getLeastIntegerValueAbovePercentage(
uint256 _total,
uint256 _percentage
)
internal
pure
returns (uint256)
{
require(_percentage > 0 && _percentage <= 100);
uint256 toAdd;
// If the total multiplied by the percentage is not divisible by 100, we need to add 1 to the result to
// compensate for the rounding down by integer division.
if ((_total * _percentage) % 100 > 0) {
toAdd = 1;
}
return (_total * _percentage) / 100 + toAdd;
}
/// @dev Differentially tests the getRequiredThreshold function against _getLeastIntegerValueAbovePercentage
function testFuzz_getRequiredThreshold_works(uint256 _numOwners, uint256 _percentage) external {
// Enforce valid percentages
uint256 percentage = bound(_percentage, 1, 100);
// Enforce a sane number of owners to keep runtime in check
uint256 numOwners = bound(_numOwners, 1, 100);
assertEq(
_getRequiredThreshold(numOwners, percentage), _getLeastIntegerValueAbovePercentage(numOwners, percentage)
);
}
/// @dev check the return values of the getRequiredThreshold function against the boundary conditions of 1 and 100
/// percent.
function testFuzz_getRequiredThreshold_atBoundaries_works(uint256 _numOwners) external {
// Enforce a sane number of owners to keep runtime in check
uint256 numOwners = bound(_numOwners, 1, 100);
assertEq(_getRequiredThreshold(numOwners, 100), numOwners);
assertEq(_getRequiredThreshold(numOwners, 1), 1);
}
/// @dev check the return values of the getRequiredThreshold function against manually
/// calculated values.
function test_get75PercentThreshold_Works() external view {
assertEq(livenessModule.get75PercentThreshold(20), 15);
assertEq(livenessModule.get75PercentThreshold(19), 15);
assertEq(livenessModule.get75PercentThreshold(18), 14);
assertEq(livenessModule.get75PercentThreshold(17), 13);
assertEq(livenessModule.get75PercentThreshold(16), 12);
assertEq(livenessModule.get75PercentThreshold(15), 12);
assertEq(livenessModule.get75PercentThreshold(14), 11);
assertEq(livenessModule.get75PercentThreshold(13), 10);
assertEq(livenessModule.get75PercentThreshold(12), 9);
assertEq(livenessModule.get75PercentThreshold(11), 9);
assertEq(livenessModule.get75PercentThreshold(10), 8);
assertEq(livenessModule.get75PercentThreshold(9), 7);
assertEq(livenessModule.get75PercentThreshold(8), 6);
assertEq(livenessModule.get75PercentThreshold(7), 6);
assertEq(livenessModule.get75PercentThreshold(6), 5);
assertEq(livenessModule.get75PercentThreshold(5), 4);
assertEq(livenessModule.get75PercentThreshold(4), 3);
assertEq(livenessModule.get75PercentThreshold(3), 3);
assertEq(livenessModule.get75PercentThreshold(2), 2);
assertEq(livenessModule.get75PercentThreshold(1), 1);
function test_getRequiredThreshold_hardcoded_works() external {
// 75% threshold
assertEq(_getRequiredThreshold(20, 75), 15);
assertEq(_getRequiredThreshold(19, 75), 15);
assertEq(_getRequiredThreshold(18, 75), 14);
assertEq(_getRequiredThreshold(17, 75), 13);
assertEq(_getRequiredThreshold(16, 75), 12);
assertEq(_getRequiredThreshold(15, 75), 12);
assertEq(_getRequiredThreshold(14, 75), 11);
assertEq(_getRequiredThreshold(13, 75), 10);
assertEq(_getRequiredThreshold(12, 75), 9);
assertEq(_getRequiredThreshold(11, 75), 9);
assertEq(_getRequiredThreshold(10, 75), 8);
assertEq(_getRequiredThreshold(9, 75), 7);
assertEq(_getRequiredThreshold(8, 75), 6);
assertEq(_getRequiredThreshold(7, 75), 6);
assertEq(_getRequiredThreshold(6, 75), 5);
assertEq(_getRequiredThreshold(5, 75), 4);
assertEq(_getRequiredThreshold(4, 75), 3);
assertEq(_getRequiredThreshold(3, 75), 3);
assertEq(_getRequiredThreshold(2, 75), 2);
assertEq(_getRequiredThreshold(1, 75), 1);
// 33% threshold
assertEq(_getRequiredThreshold(20, 33), 7);
assertEq(_getRequiredThreshold(19, 33), 7);
assertEq(_getRequiredThreshold(18, 33), 6);
assertEq(_getRequiredThreshold(17, 33), 6);
assertEq(_getRequiredThreshold(16, 33), 6);
assertEq(_getRequiredThreshold(15, 33), 5);
assertEq(_getRequiredThreshold(14, 33), 5);
assertEq(_getRequiredThreshold(13, 33), 5);
assertEq(_getRequiredThreshold(12, 33), 4);
assertEq(_getRequiredThreshold(11, 33), 4);
assertEq(_getRequiredThreshold(10, 33), 4);
assertEq(_getRequiredThreshold(9, 33), 3);
assertEq(_getRequiredThreshold(8, 33), 3);
assertEq(_getRequiredThreshold(7, 33), 3);
assertEq(_getRequiredThreshold(6, 33), 2);
assertEq(_getRequiredThreshold(5, 33), 2);
assertEq(_getRequiredThreshold(4, 33), 2);
assertEq(_getRequiredThreshold(3, 33), 1);
assertEq(_getRequiredThreshold(2, 33), 1);
assertEq(_getRequiredThreshold(1, 33), 1);
}
}
......@@ -275,7 +365,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
);
_warpPastLivenessInterval();
vm.expectRevert("LivenessModule: Safe must have a threshold of 75% of the number of owners");
vm.expectRevert("LivenessModule: Insufficient threshold for the number of owners");
livenessModule.removeOwners(prevOwners, ownersToRemove);
}
}
......@@ -325,11 +415,6 @@ contract LivenessModule_RemoveOwners_Test is LivenessModule_TestInit {
}
}
/// @dev A copy of LivenessModule.get75PercentThreshold as a free function to use below.
function get75PercentThreshold(uint256 _numOwners) pure returns (uint256 threshold_) {
threshold_ = (_numOwners * 75 + 99) / 100;
}
contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
using SafeTestLib for SafeInstance;
......@@ -349,7 +434,7 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
/// @dev This contract inherits the storage layout from the LivenessModule_TestInit contract, but we
/// override the base setUp function, to avoid instantiating an unnecessary Safe and liveness checking system.
function setUp() public override {
vm.warp(initTime);
vm.warp(INIT_TIME);
fallbackOwner = makeAddr("fallbackOwner");
}
......@@ -357,10 +442,11 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
function _prepare(
uint256 _numOwners,
uint256 _minOwners,
uint256 _numLiveOwners
uint256 _numLiveOwners,
uint256 _thresholdPercentage
)
internal
returns (uint256 numOwners_, uint256 minOwners_, uint256 numLiveOwners_)
returns (uint256 numOwners_, uint256 minOwners_, uint256 numLiveOwners_, uint256 thresholdPercentage_)
{
// First we modify the test parameters to ensure that they describe a plausible starting point.
//
......@@ -374,6 +460,9 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
// Ensure that _numLiveOwners is less than _numOwners so that we can remove at least one owner.
numLiveOwners_ = bound(_numLiveOwners, 0, numOwners_ - 1);
// thresholdPercentage must be between a percentage greater than 0.
thresholdPercentage_ = bound(_thresholdPercentage, 1, 100);
// The above bounds are a bit tricky, so we assert that the resulting parameters enable us to test all possible
// success and revert cases in the removeOwners function.
// This is also necessary to avoid underflows or out of bounds accesses in the test.
......@@ -385,13 +474,14 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
// Create a Safe with _numOwners owners
(, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("rmOwnersTest", numOwners_);
uint256 threshold = get75PercentThreshold(numOwners_);
uint256 threshold = _getRequiredThreshold(numOwners_, thresholdPercentage_);
safeInstance = _setupSafe(keys, threshold);
livenessGuard = new LivenessGuard(safeInstance.safe);
livenessModule = new LivenessModule({
_safe: safeInstance.safe,
_livenessGuard: livenessGuard,
_livenessInterval: livenessInterval,
_livenessInterval: LIVENESS_INTERVAL,
_thresholdPercentage: thresholdPercentage_,
_minOwners: minOwners_,
_fallbackOwner: fallbackOwner
});
......@@ -407,13 +497,15 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
uint256 _numOwners,
uint256 _minOwners,
uint256 _numLiveOwners,
uint256 _thresholdPercentage,
uint256 _shutDownBehavior,
uint256 _numOwnersToRemoveinShutDown
)
external
{
// Prepare the test env and test params
(uint256 numOwners, uint256 minOwners, uint256 numLiveOwners) = _prepare(_numOwners, _minOwners, _numLiveOwners);
(uint256 numOwners, uint256 minOwners, uint256 numLiveOwners, uint256 thresholdPercentage) =
_prepare(_numOwners, _minOwners, _numLiveOwners, _thresholdPercentage);
// Create an array of live owners, and call showLiveness for each of them
address[] memory liveOwners = new address[](numLiveOwners);
......@@ -437,7 +529,7 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
// Validate the resulting state of the Safe
assertEq(safeInstance.safe.getOwners().length, numLiveOwners);
assertEq(safeInstance.safe.getThreshold(), get75PercentThreshold(numLiveOwners));
assertEq(safeInstance.safe.getThreshold(), _getRequiredThreshold(numLiveOwners, thresholdPercentage));
for (uint256 i; i < numLiveOwners; i++) {
assertTrue(safeInstance.safe.isOwner(liveOwners[i]));
}
......@@ -507,7 +599,7 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
}
// For both of the incorrect behaviors, verify no change to the Safe state
assertEq(safeInstance.safe.getOwners().length, numOwners);
assertEq(safeInstance.safe.getThreshold(), get75PercentThreshold(numOwners));
assertEq(safeInstance.safe.getThreshold(), _getRequiredThreshold(numOwners, thresholdPercentage));
for (uint256 i; i < numOwners; i++) {
assertTrue(safeInstance.safe.isOwner(safeInstance.owners[i]));
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment