From 9c170c39bc18dd43de1f709c533ea89d96e77725 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Thu, 17 Oct 2024 02:04:57 +0400 Subject: [PATCH 1/3] Update Tiebreaker.isTie() to match the mechanics doc --- contracts/interfaces/ISealable.sol | 1 - contracts/libraries/SealableCalls.sol | 89 ++++-------- contracts/libraries/Tiebreaker.sol | 38 ++++-- test/mocks/GateSealMock.sol | 37 ----- test/mocks/SealableMock.sol | 20 +-- test/unit/DualGovernance.t.sol | 12 +- test/unit/libraries/SealableCalls.t.sol | 171 +++++++++++++++++------- test/unit/libraries/Tiebreaker.t.sol | 154 ++++++++++++++++----- 8 files changed, 313 insertions(+), 209 deletions(-) delete mode 100644 test/mocks/GateSealMock.sol diff --git a/contracts/interfaces/ISealable.sol b/contracts/interfaces/ISealable.sol index 4d616d80..263a29ec 100644 --- a/contracts/interfaces/ISealable.sol +++ b/contracts/interfaces/ISealable.sol @@ -4,6 +4,5 @@ pragma solidity 0.8.26; interface ISealable { function resume() external; function pauseFor(uint256 duration) external; - function isPaused() external view returns (bool); function getResumeSinceTimestamp() external view returns (uint256); } diff --git a/contracts/libraries/SealableCalls.sol b/contracts/libraries/SealableCalls.sol index db2738c5..04cd1e5c 100644 --- a/contracts/libraries/SealableCalls.sol +++ b/contracts/libraries/SealableCalls.sol @@ -3,71 +3,40 @@ pragma solidity 0.8.26; import {ISealable} from "../interfaces/ISealable.sol"; -/// @title SealableCalls Library -/// @dev A library for making calls to a contract implementing the ISealable interface. -library SealableCalls { - /// @dev Calls the `pauseFor` function on a `Sealable` contract with the specified `sealDuration`. - /// If the call is successful and the contract is paused, it returns `true` and low-level error message, if any. - /// If the call fails, it returns `false` and the low-level error message. - /// - /// @param sealable The `Sealable` contract to call the `pauseFor` function on. - /// @param sealDuration The duration for which the contract should be paused. - /// - /// @return success A boolean indicating whether the call to `pauseFor` was successful and the contract is paused. - /// @return lowLevelError The low-level error message, if any, encountered during the call to `pauseFor`. - function callPauseFor( - ISealable sealable, - uint256 sealDuration - ) internal returns (bool success, bytes memory lowLevelError) { - try sealable.pauseFor(sealDuration) { - (bool isPausedCallSuccess, bytes memory isPausedLowLevelError, bool isPaused) = callIsPaused(sealable); - success = isPausedCallSuccess && isPaused; - lowLevelError = isPausedLowLevelError; - } catch (bytes memory pauseForLowLevelError) { - success = false; - lowLevelError = pauseForLowLevelError; - } - } +/// @dev All calls to sealable addresses less than MIN_VALID_SEALABLE_ADDRESS are treated as unsuccessful +/// to prevent potential false positives for current or future precompiled addresses. +address constant MIN_VALID_SEALABLE_ADDRESS = address(1024); - /// @dev Calls the `isPaused` function on a `Sealable` contract to check if the contract is currently paused. - /// If the call is successful, it returns `true` indicating that the contract is paused, along with a low-level error message if any. - /// If the call fails, it returns `false` and the low-level error message encountered during the call. - /// - /// @param sealable The `Sealable` contract to call the `isPaused` function on. - /// - /// @return success A boolean indicating whether the call to `isPaused` was successful. - /// @return lowLevelError The low-level error message, if any, encountered during the call to `isPaused`. - /// @return isPaused A boolean indicating whether the contract is currently paused. - function callIsPaused(ISealable sealable) - internal +library SealableCalls { + /// @notice Attempts to call `ISealable.getResumeSinceTimestamp()` method, returning whether the call succeeded + /// and the result of the `ISealable.getResumeSinceTimestamp()` call if it succeeded. + /// @dev Performs a static call to the `getResumeSinceTimestamp()` method on the `ISealable` interface. + /// Ensures that the function does not revert even if the `sealable` contract does not implement + /// the interface, has no code at the address, or returns unexpected data. + /// Calls to addresses less than `MIN_VALID_SEALABLE_ADDRESS` are treated as unsuccessful to prevent + /// potential false positives from current or future precompiled addresses. + /// @param sealable The address of the sealable contract to check. + /// @return success Indicates whether the call to `getResumeSinceTimestamp()` was successful. + /// @return resumeSinceTimestamp The timestamp when the contract is expected to become unpaused. + /// If the value is less than `block.timestamp`, it indicates the contract resumed in the past; + /// if `type(uint256).max`, the contract is paused indefinitely. + function callGetResumeSinceTimestamp(address sealable) + external view - returns (bool success, bytes memory lowLevelError, bool isPaused) + returns (bool success, uint256 resumeSinceTimestamp) { - try sealable.isPaused() returns (bool isPausedResult) { - success = true; - isPaused = isPausedResult; - } catch (bytes memory isPausedLowLevelError) { - success = false; - lowLevelError = isPausedLowLevelError; + if (sealable < MIN_VALID_SEALABLE_ADDRESS) { + return (false, 0); } - } - /// @dev Calls the `resume` function on a `Sealable` contract to resume the contract's functionality. - /// If the call is successful and the contract is resumed, it returns `true` and a low-level error message, if any. - /// If the call fails, it returns `false` and the low-level error message encountered during the call. - /// - /// @param sealable The `Sealable` contract to call the `resume` function on. - /// - /// @return success A boolean indicating whether the call to `resume` was successful and the contract is resumed. - /// @return lowLevelError The low-level error message, if any, encountered during the call to `resume`. - function callResume(ISealable sealable) internal returns (bool success, bytes memory lowLevelError) { - try sealable.resume() { - (bool isPausedCallSuccess, bytes memory isPausedLowLevelError, bool isPaused) = callIsPaused(sealable); - success = isPausedCallSuccess && !isPaused; - lowLevelError = isPausedLowLevelError; - } catch (bytes memory resumeLowLevelError) { - success = false; - lowLevelError = resumeLowLevelError; + // Low-level call to the `getResumeSinceTimestamp` function on the `sealable` contract + (bool isCallSucceed, bytes memory returndata) = + sealable.staticcall(abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector)); + + // Check if the call succeeded and returned the expected data length (32 bytes, single uint256) + if (isCallSucceed && returndata.length == 32) { + success = true; + resumeSinceTimestamp = abi.decode(returndata, (uint256)); } } } diff --git a/contracts/libraries/Tiebreaker.sol b/contracts/libraries/Tiebreaker.sol index bf775705..13240fbb 100644 --- a/contracts/libraries/Tiebreaker.sol +++ b/contracts/libraries/Tiebreaker.sol @@ -6,11 +6,11 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet import {Duration} from "../types/Duration.sol"; import {Timestamp, Timestamps} from "../types/Duration.sol"; -import {ISealable} from "../interfaces/ISealable.sol"; import {ITiebreaker} from "../interfaces/ITiebreaker.sol"; import {IDualGovernance} from "../interfaces/IDualGovernance.sol"; -import {SealableCalls} from "./SealableCalls.sol"; +import {SealableCalls} from "../libraries/SealableCalls.sol"; + import {State as DualGovernanceState} from "./DualGovernanceStateMachine.sol"; /// @title Tiebreaker Library @@ -20,7 +20,6 @@ import {State as DualGovernanceState} from "./DualGovernanceStateMachine.sol"; /// the power to execute pending proposals, bypassing the DG dynamic timelock, and unpause any /// protocol contract under the specific conditions of the deadlock. library Tiebreaker { - using SealableCalls for ISealable; using EnumerableSet for EnumerableSet.AddressSet; error TiebreakNotAllowed(); @@ -68,7 +67,8 @@ library Tiebreaker { if (sealableWithdrawalBlockersCount == maxSealableWithdrawalBlockersCount) { revert SealableWithdrawalBlockersLimitReached(); } - (bool isCallSucceed, /* lowLevelError */, /* isPaused */ ) = ISealable(sealableWithdrawalBlocker).callIsPaused(); + (bool isCallSucceed, /* resumeSinceTimestamp */ ) = + SealableCalls.callGetResumeSinceTimestamp(sealableWithdrawalBlocker); if (!isCallSucceed) { revert InvalidSealable(sealableWithdrawalBlocker); } @@ -172,19 +172,31 @@ library Tiebreaker { return true; } - return state == DualGovernanceState.RageQuit && isSomeSealableWithdrawalBlockerPaused(self); + return state == DualGovernanceState.RageQuit && isSomeSealableWithdrawalBlockerPausedForLongTermOrFaulty(self); } - /// @notice Checks if any sealable withdrawal blocker is paused. - /// @param self The context storage. - /// @return True if any sealable withdrawal blocker is paused, false otherwise. - function isSomeSealableWithdrawalBlockerPaused(Context storage self) internal view returns (bool) { + /// @notice Checks if any sealable withdrawal blocker has been paused for a duration that exceeds + /// `tiebreakerActivationTimeout`, or if it is functioning improperly. + /// @param self The context containing the sealable withdrawal blockers. + /// @return True if any sealable withdrawal blocker is paused for a duration exceeding `tiebreakerActivationTimeout` + /// or is functioning incorrectly, false otherwise. + function isSomeSealableWithdrawalBlockerPausedForLongTermOrFaulty(Context storage self) + internal + view + returns (bool) + { uint256 sealableWithdrawalBlockersCount = self.sealableWithdrawalBlockers.length(); - for (uint256 i = 0; i < sealableWithdrawalBlockersCount; ++i) { - (bool isCallSucceed, /* lowLevelError */, bool isPaused) = - ISealable(self.sealableWithdrawalBlockers.at(i)).callIsPaused(); + uint256 tiebreakAllowedTillTimestampInSeconds = + self.tiebreakerActivationTimeout.addTo(Timestamps.now()).toSeconds(); - if (isPaused || !isCallSucceed) return true; + for (uint256 i = 0; i < sealableWithdrawalBlockersCount; ++i) { + (bool isCallSucceed, uint256 resumeSinceTimestampInSeconds) = + SealableCalls.callGetResumeSinceTimestamp(self.sealableWithdrawalBlockers.at(i)); + /// @dev If the call failed, mark the sealable as faulty. + /// Otherwise, consider it paused if its pause duration exceeds `tiebreakerActivationTimeout`. + if (!isCallSucceed || tiebreakAllowedTillTimestampInSeconds <= resumeSinceTimestampInSeconds) { + return true; + } } return false; } diff --git a/test/mocks/GateSealMock.sol b/test/mocks/GateSealMock.sol deleted file mode 100644 index e926a572..00000000 --- a/test/mocks/GateSealMock.sol +++ /dev/null @@ -1,37 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.26; - -import {ISealable} from "contracts/interfaces/ISealable.sol"; -import {IGateSeal} from "contracts/interfaces/IGateSeal.sol"; - -contract GateSealMock is IGateSeal { - error GateSealExpired(); - - event SealablesSealed(address[] sealables); - - uint256 internal constant _INFINITE_DURATION = type(uint256).max; - - uint256 internal _expiryTimestamp; - uint256 internal _seal_duration_seconds; - address[] internal _sealedSealables; - - constructor(uint256 sealDurationSeconds, uint256 lifetime) { - _seal_duration_seconds = sealDurationSeconds; - _expiryTimestamp = block.timestamp + lifetime; - } - - function seal(address[] calldata sealables) external { - if (_expiryTimestamp <= block.timestamp) { - revert GateSealExpired(); - } - _sealedSealables = sealables; - _expiryTimestamp = block.timestamp; - - for (uint256 i = 0; i < sealables.length; ++i) { - ISealable(sealables[i]).pauseFor(_seal_duration_seconds); - assert(ISealable(sealables[i]).isPaused()); - } - - emit SealablesSealed(sealables); - } -} diff --git a/test/mocks/SealableMock.sol b/test/mocks/SealableMock.sol index 73da6af8..03d252ed 100644 --- a/test/mocks/SealableMock.sol +++ b/test/mocks/SealableMock.sol @@ -4,13 +4,16 @@ pragma solidity 0.8.26; import {ISealable} from "contracts/interfaces/ISealable.sol"; contract SealableMock is ISealable { + uint256 public constant PAUSE_INFINITELY = type(uint256).max; + bool private paused; bool private shouldRevertPauseFor; bool private shouldRevertIsPaused; bool private shouldRevertResume; + uint256 private _resumeSinceTimestamp; function getResumeSinceTimestamp() external view override returns (uint256) { - revert("Not implemented"); + return _resumeSinceTimestamp; } function setShouldRevertPauseFor(bool _shouldRevert) external { @@ -25,24 +28,21 @@ contract SealableMock is ISealable { shouldRevertResume = _shouldRevert; } - function pauseFor(uint256) external override { + function pauseFor(uint256 duration) external override { if (shouldRevertPauseFor) { revert("pauseFor failed"); } - paused = true; - } - - function isPaused() external view override returns (bool) { - if (shouldRevertIsPaused) { - revert("isPaused failed"); + if (duration == PAUSE_INFINITELY) { + _resumeSinceTimestamp = PAUSE_INFINITELY; + } else { + _resumeSinceTimestamp = block.timestamp + duration; } - return paused; } function resume() external override { if (shouldRevertResume) { revert("resume failed"); } - paused = false; + _resumeSinceTimestamp = block.timestamp; } } diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index 36a6601c..2a76b3ce 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -1798,8 +1798,12 @@ contract DualGovernanceUnitTests is UnitTest { assertEq(_dualGovernance.getEffectiveState(), State.RageQuit); assertFalse(_dualGovernance.getTiebreakerDetails().isTie); - // Simulate the sealable withdrawal blocker was paused - vm.mockCall(address(sealable), abi.encodeWithSelector(SealableMock.isPaused.selector), abi.encode(true)); + // Simulate the sealable withdrawal blocker was paused indefinitely + vm.mockCall( + address(sealable), + abi.encodeWithSelector(SealableMock.getResumeSinceTimestamp.selector), + abi.encode(type(uint256).max) + ); assertEq(_dualGovernance.getPersistedState(), State.VetoSignalling); assertEq(_dualGovernance.getEffectiveState(), State.RageQuit); @@ -1813,7 +1817,9 @@ contract DualGovernanceUnitTests is UnitTest { assertTrue(_dualGovernance.getTiebreakerDetails().isTie); // Return sealable to unpaused state for further testing - vm.mockCall(address(sealable), abi.encodeWithSelector(SealableMock.isPaused.selector), abi.encode(false)); + vm.mockCall( + address(sealable), abi.encodeWithSelector(SealableMock.getResumeSinceTimestamp.selector), abi.encode(0) + ); assertFalse(_dualGovernance.getTiebreakerDetails().isTie); _wait(tiebreakerActivationTimeout); diff --git a/test/unit/libraries/SealableCalls.t.sol b/test/unit/libraries/SealableCalls.t.sol index 12a8771d..42309459 100644 --- a/test/unit/libraries/SealableCalls.t.sol +++ b/test/unit/libraries/SealableCalls.t.sol @@ -1,80 +1,147 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.26; -import {SealableCalls} from "contracts/libraries/SealableCalls.sol"; +import {stdError} from "forge-std/StdError.sol"; +import {ISealable, SealableCalls, MIN_VALID_SEALABLE_ADDRESS} from "contracts/libraries/SealableCalls.sol"; import {UnitTest} from "test/utils/unit-test.sol"; import {SealableMock} from "test/mocks/SealableMock.sol"; -contract SealableCallsUnitTests is UnitTest { - SealableMock private _sealableMock; - - function setUp() public { - _sealableMock = new SealableMock(); +error CustomSealableError(string message); + +contract SealableCallsTest is UnitTest { + address private immutable _SEALABLE = makeAddr("SEALABLE"); + + function test_callGetResumeSinceTimestamp_IsCallSucceedTrue() external { + // edge case when timestamp is 0 + _mockSealableResumeSinceTimestampResult(_SEALABLE, 0); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: true, resumeSinceTimestamp: 0}); + + // edge case when sealable paused indefinitely + _mockSealableResumeSinceTimestampResult(_SEALABLE, type(uint256).max); + _assertGetResumeSinceTimestampCallResult({ + sealable: _SEALABLE, + isCallSucceed: true, + resumeSinceTimestamp: type(uint256).max + }); + + // paused for finite time + uint256 sealablePauseDuration = 14 days; + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp + sealablePauseDuration); + _assertGetResumeSinceTimestampCallResult({ + sealable: _SEALABLE, + isCallSucceed: true, + resumeSinceTimestamp: block.timestamp + sealablePauseDuration + }); } - function testCallPauseForSuccess() public { - (bool success, bytes memory lowLevelError) = SealableCalls.callPauseFor(_sealableMock, 1 days); - - assertTrue(success); - assertEq(lowLevelError.length, 0); - - (bool isPausedSuccess,, bool isPaused) = SealableCalls.callIsPaused(_sealableMock); - assertTrue(isPausedSuccess); - assertTrue(isPaused); + function testFuzz_callGetResumeSinceTimestamp_IsCallSucceedTrue(uint256 resumeSinceTimestamp) external { + _mockSealableResumeSinceTimestampResult(_SEALABLE, resumeSinceTimestamp); + _assertGetResumeSinceTimestampCallResult({ + sealable: _SEALABLE, + isCallSucceed: true, + resumeSinceTimestamp: resumeSinceTimestamp + }); } - function testCallPauseForFailure() public { - _sealableMock.setShouldRevertPauseFor(true); - - (bool success, bytes memory lowLevelError) = SealableCalls.callPauseFor(_sealableMock, 1 days); - bytes memory expectedError = abi.encodeWithSignature("Error(string)", "pauseFor failed"); - - assertFalse(success); - assertEq(keccak256(lowLevelError), keccak256(expectedError)); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableIsNotContract() external { + assertEq(_SEALABLE.code.length, 0); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); } - function testCallIsPausedSuccess() public { - SealableCalls.callPauseFor(_sealableMock, 1 days); - - (bool success, bytes memory lowLevelError, bool isPaused) = SealableCalls.callIsPaused(_sealableMock); - - assertTrue(success); - assertTrue(isPaused); - assertEq(lowLevelError.length, 0); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableRevertWithoutErrorReason() external { + _mockSealableResumeSinceTimestampReverts(_SEALABLE, ""); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); } - function testCallIsPausedFailure() public { - _sealableMock.setShouldRevertIsPaused(true); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableRevertWithStandardError() external { + _mockSealableResumeSinceTimestampReverts(_SEALABLE, stdError.divisionError); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); + } - (bool success, bytes memory lowLevelError, bool isPaused) = SealableCalls.callIsPaused(_sealableMock); - bytes memory expectedError = abi.encodeWithSignature("Error(string)", "isPaused failed"); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableRevertWithStringError() external { + _mockSealableResumeSinceTimestampReverts(_SEALABLE, "ERROR_MESSAGE"); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); + } - assertFalse(success); - assertFalse(isPaused); - assertEq(keccak256(lowLevelError), keccak256(expectedError)); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableRevertWithCustomError() external { + _mockSealableResumeSinceTimestampReverts( + _SEALABLE, abi.encodeWithSelector(CustomSealableError.selector, "error reason") + ); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); } - function testCallResumeSuccess() public { - SealableCalls.callPauseFor(_sealableMock, 1 days); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableReturnsInvalidResultDynamicLength() + external + { + string[] memory customResult = new string[](2); + customResult[0] = "Hello"; + customResult[1] = "World"; + + vm.mockCall( + _SEALABLE, abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector), abi.encode(customResult) + ); + _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); + } - (bool success, bytes memory lowLevelError) = SealableCalls.callResume(_sealableMock); + function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableLessThanMinValidSealableAddress() external { + // check addresses (0x00, 0x400) reserved for precompiles. + // Currently Ethereum has only precompiles for 0x01 - 0x09 but new precompiles may be added in the future + for (uint256 i = 0; i < uint160(MIN_VALID_SEALABLE_ADDRESS); ++i) { + address precompile = address(uint160(i)); + _assertGetResumeSinceTimestampCallResult({ + sealable: precompile, + isCallSucceed: false, + resumeSinceTimestamp: 0 + }); + } + + address LAST_INVALID_SEALABLE_ADDRESS = address(uint160(MIN_VALID_SEALABLE_ADDRESS) - 1); + // assuming address MIN_VALID_SEALABLE_ADDRESS and LAST_INVALID_SEALABLE_ADDRESS has + // deployed bytecode which returns some value + _mockSealableResumeSinceTimestampResult(MIN_VALID_SEALABLE_ADDRESS, block.timestamp); + _mockSealableResumeSinceTimestampResult(LAST_INVALID_SEALABLE_ADDRESS, block.timestamp); + + // call to the LAST_INVALID_SEALABLE_ADDRESS considered not succeed as made to precompile + _assertGetResumeSinceTimestampCallResult({ + sealable: LAST_INVALID_SEALABLE_ADDRESS, + isCallSucceed: false, + resumeSinceTimestamp: 0 + }); + + // but call to the MIN_VALID_SEALABLE_ADDRESS considered succeed + _assertGetResumeSinceTimestampCallResult({ + sealable: MIN_VALID_SEALABLE_ADDRESS, + isCallSucceed: true, + resumeSinceTimestamp: block.timestamp + }); + } - assertTrue(success); - assertEq(lowLevelError.length, 0); + // --- + // Helper Test Methods + // --- - (bool isPausedSuccess,, bool isPaused) = SealableCalls.callIsPaused(_sealableMock); - assertTrue(isPausedSuccess); - assertFalse(isPaused); + function _mockSealableResumeSinceTimestampResult(address sealable, uint256 resumeSinceTimestamp) internal { + vm.mockCall( + sealable, + abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector), + abi.encode(resumeSinceTimestamp) + ); } - function testCallResumeFailure() public { - _sealableMock.setShouldRevertResume(true); + function _assertGetResumeSinceTimestampCallResult( + address sealable, + bool isCallSucceed, + uint256 resumeSinceTimestamp + ) internal { + (bool isCallSucceedActual, uint256 resumeSinceTimestampActual) = + SealableCalls.callGetResumeSinceTimestamp(sealable); - (bool success, bytes memory lowLevelError) = SealableCalls.callResume(_sealableMock); - bytes memory expectedError = abi.encodeWithSignature("Error(string)", "resume failed"); + assertEq(isCallSucceedActual, isCallSucceed, "Unexpected isCallSucceed value"); + assertEq(resumeSinceTimestampActual, resumeSinceTimestamp, "Unexpected resumeSinceTimestamp value"); + } - assertFalse(success); - assertEq(keccak256(lowLevelError), keccak256(expectedError)); + function _mockSealableResumeSinceTimestampReverts(address sealable, bytes memory revertReason) internal { + vm.mockCallRevert(sealable, abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector), revertReason); } } diff --git a/test/unit/libraries/Tiebreaker.t.sol b/test/unit/libraries/Tiebreaker.t.sol index f51fe839..23ccf8e3 100644 --- a/test/unit/libraries/Tiebreaker.t.sol +++ b/test/unit/libraries/Tiebreaker.t.sol @@ -14,23 +14,30 @@ import {UnitTest} from "test/utils/unit-test.sol"; import {SealableMock} from "../../mocks/SealableMock.sol"; contract TiebreakerTest is UnitTest { + using Tiebreaker for Tiebreaker.Context; using EnumerableSet for EnumerableSet.AddressSet; + address private immutable _SEALABLE = makeAddr("SEALABLE"); Tiebreaker.Context private context; + SealableMock private mockSealable1; SealableMock private mockSealable2; function setUp() external { mockSealable1 = new SealableMock(); mockSealable2 = new SealableMock(); + + // The expected state of the sealable most of the time - unpaused + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp - 1); } function test_addSealableWithdrawalBlocker_HappyPath() external { vm.expectEmit(); - emit Tiebreaker.SealableWithdrawalBlockerAdded(address(mockSealable1)); - Tiebreaker.addSealableWithdrawalBlocker(context, address(mockSealable1), 1); + emit Tiebreaker.SealableWithdrawalBlockerAdded(_SEALABLE); - assertTrue(context.sealableWithdrawalBlockers.contains(address(mockSealable1))); + this.external__addSealableWithdrawalBlocker(_SEALABLE); + + assertTrue(context.sealableWithdrawalBlockers.contains(_SEALABLE)); } function test_addSealableWithdrawalBlocker_RevertOn_LimitReached() external { @@ -50,15 +57,25 @@ contract TiebreakerTest is UnitTest { } function test_addSealableWithdrawalBlocker_RevertOn_InvalidSealable() external { - mockSealable1.setShouldRevertIsPaused(true); + address emptyAccount = makeAddr("EMPTY_ACCOUNT"); + assertEq(emptyAccount.code.length, 0); - vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, address(mockSealable1))); - // external call should be used to intercept the revert - this.external__addSealableWithdrawalBlocker(address(mockSealable1), 2); + // revert when sealable is not a contract + vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, emptyAccount)); - vm.expectRevert(); - // external call should be used to intercept the revert - this.external__addSealableWithdrawalBlocker(address(0x123), 2); + this.external__addSealableWithdrawalBlocker(emptyAccount); + + // reverts when sealable's isPaused call reverts without reason + _mockSealableResumeSinceTimestampReverts(_SEALABLE, ""); + vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, _SEALABLE)); + + this.external__addSealableWithdrawalBlocker(_SEALABLE); + + // revert when sealable's isPaused call returns invalid value + _mockSealableResumeSinceTimestampReverts(_SEALABLE, abi.encode("Invalid Result")); + vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, _SEALABLE)); + + this.external__addSealableWithdrawalBlocker(_SEALABLE); } function test_removeSealableWithdrawalBlocker_HappyPath() external { @@ -130,37 +147,68 @@ contract TiebreakerTest is UnitTest { Tiebreaker.setTiebreakerActivationTimeout(context, minTimeout, newTimeout, maxTimeout); } - function test_isSomeSealableWithdrawalBlockerPaused_HappyPath() external { - mockSealable1.pauseFor(1 days); - Tiebreaker.addSealableWithdrawalBlocker(context, address(mockSealable1), 2); + // --- + // checkTie() + // --- - bool result = Tiebreaker.isSomeSealableWithdrawalBlockerPaused(context); - assertTrue(result); + function test_checkTie_HappyPath_SealablePausedInRageQuitState() external { + Timestamp normalOrVetoCooldownExitedAt = Timestamps.now(); + Duration tiebreakerActivationTimeout = Durations.from(180 days); + Timestamp tiebreakerAllowedAt = tiebreakerActivationTimeout.addTo(normalOrVetoCooldownExitedAt); - mockSealable1.resume(); + this.external__addSealableWithdrawalBlocker(_SEALABLE); + this.external__setTiebreakerActivationTimeout(tiebreakerActivationTimeout); - result = Tiebreaker.isSomeSealableWithdrawalBlockerPaused(context); - assertFalse(result); + // tiebreak is not allowed in the normal state + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.Normal, normalOrVetoCooldownExitedAt); - mockSealable1.setShouldRevertIsPaused(true); + _mockSealableResumeSinceTimestampResult(_SEALABLE, type(uint256).max); - result = Tiebreaker.isSomeSealableWithdrawalBlockerPaused(context); - assertTrue(result); - } + // tiebreak is not allowed in the state different from RageQuit even when some sealable is blocked + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.VetoSignalling, normalOrVetoCooldownExitedAt); - function test_checkTie_HappyPath() external { - Timestamp cooldownExitedAt = Timestamps.from(block.timestamp); + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.VetoCooldown, normalOrVetoCooldownExitedAt); - Tiebreaker.addSealableWithdrawalBlocker(context, address(mockSealable1), 1); - Tiebreaker.setTiebreakerActivationTimeout( - context, Duration.wrap(1 days), Duration.wrap(3 days), Duration.wrap(10 days) + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.VetoSignallingDeactivation, normalOrVetoCooldownExitedAt); + + // no revert, tiebreak is allowed + // tiebreak is allowed when some sealable is paused for a duration exceeded tiebreakerActivationTimeout + // and the DG in the RageQuit state + this.external__checkTie(DualGovernanceState.RageQuit, normalOrVetoCooldownExitedAt); + + // simulate tiebreaker was locked for time < tiebreakerActivationTimeout + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp + 14 days); + + // then tiebreaker activation is not allowed + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.VetoSignallingDeactivation, normalOrVetoCooldownExitedAt); + + // check edge case when resumeSinceTimestamp == block.timestamp + tiebreakerActivationTimeout + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp + tiebreakerActivationTimeout.toSeconds()); + + // tiebreaker activation should be allowed + this.external__checkTie(DualGovernanceState.RageQuit, normalOrVetoCooldownExitedAt); + + // but when resumeSinceTimestamp == block.timestamp + tiebreakerActivationTimeout - 1 + _mockSealableResumeSinceTimestampResult( + _SEALABLE, block.timestamp + tiebreakerActivationTimeout.toSeconds() - 1 ); - mockSealable1.pauseFor(1 days); - Tiebreaker.checkTie(context, DualGovernanceState.RageQuit, cooldownExitedAt); + // tiebreaker activation is not allowed + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + this.external__checkTie(DualGovernanceState.RageQuit, normalOrVetoCooldownExitedAt); + + // if the DG locked for a long period of time, tiebreak becomes allowed in any state + _wait(tiebreakerActivationTimeout); + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp); + assertTrue(Timestamps.now() >= tiebreakerAllowedAt); - _wait(Duration.wrap(3 days)); - Tiebreaker.checkTie(context, DualGovernanceState.VetoSignalling, cooldownExitedAt); + // call does not revert + this.external__checkTie(DualGovernanceState.VetoSignalling, normalOrVetoCooldownExitedAt); } function test_checkTie_RevertOn_NormalOrVetoCooldownState() external { @@ -308,11 +356,51 @@ contract TiebreakerTest is UnitTest { Tiebreaker.checkCallerIsTiebreakerCommittee(context); } - function external__addSealableWithdrawalBlocker(address sealable, uint256 count) external { - Tiebreaker.addSealableWithdrawalBlocker(context, sealable, count); + // overloaded method, to not pass limit parameter each time in the tests + function external__addSealableWithdrawalBlocker(address sealable) external { + context.addSealableWithdrawalBlocker(sealable, type(uint256).max); + } + + function external__addSealableWithdrawalBlocker( + address sealable, + uint256 maxSealableWithdrawalBlockersCount + ) external { + context.addSealableWithdrawalBlocker(sealable, maxSealableWithdrawalBlockersCount); } function external__removeSealableWithdrawalBlocker(address sealable) external { Tiebreaker.removeSealableWithdrawalBlocker(context, sealable); } + + function external__setTiebreakerActivationTimeout( + Duration minTimeout, + Duration timeout, + Duration maxTimeout + ) external { + context.setTiebreakerActivationTimeout(minTimeout, timeout, maxTimeout); + } + + function external__setTiebreakerActivationTimeout(Duration timeout) external { + context.setTiebreakerActivationTimeout(Durations.from(0), timeout, Durations.from(365 days)); + } + + function external__checkTie(DualGovernanceState dgState, Timestamp normalOrVetoCooldownExitedAt) external { + context.checkTie(dgState, normalOrVetoCooldownExitedAt); + } + + // --- + // Internal Testing Helper Methods + // --- + + function _mockSealableResumeSinceTimestampResult(address sealable, uint256 resumeSinceTimestamp) internal { + vm.mockCall( + sealable, + abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector), + abi.encode(resumeSinceTimestamp) + ); + } + + function _mockSealableResumeSinceTimestampReverts(address sealable, bytes memory revertReason) internal { + vm.mockCallRevert(sealable, abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector), revertReason); + } } From a8b4ab95b9f61320c4e1f0171925b4b06aab6a9e Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Mon, 21 Oct 2024 02:39:13 +0400 Subject: [PATCH 2/3] Better comments and naming. Use strict time check --- contracts/libraries/Tiebreaker.sol | 15 +++++++++------ test/unit/libraries/Tiebreaker.t.sol | 10 +++++----- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/contracts/libraries/Tiebreaker.sol b/contracts/libraries/Tiebreaker.sol index 13240fbb..87efe991 100644 --- a/contracts/libraries/Tiebreaker.sol +++ b/contracts/libraries/Tiebreaker.sol @@ -175,7 +175,7 @@ library Tiebreaker { return state == DualGovernanceState.RageQuit && isSomeSealableWithdrawalBlockerPausedForLongTermOrFaulty(self); } - /// @notice Checks if any sealable withdrawal blocker has been paused for a duration that exceeds + /// @notice Determines whether any sealable withdrawal blocker has been paused for a duration exceeding /// `tiebreakerActivationTimeout`, or if it is functioning improperly. /// @param self The context containing the sealable withdrawal blockers. /// @return True if any sealable withdrawal blocker is paused for a duration exceeding `tiebreakerActivationTimeout` @@ -186,15 +186,18 @@ library Tiebreaker { returns (bool) { uint256 sealableWithdrawalBlockersCount = self.sealableWithdrawalBlockers.length(); - uint256 tiebreakAllowedTillTimestampInSeconds = + + /// @dev If a sealable has been paused for less than or equal to the `tiebreakerActivationTimeout` duration, + /// counting from the current `block.timestamp`, it is not considered paused for the "long term", and the + /// tiebreaker committee is not permitted to unpause it. + uint256 tiebreakAllowedAfterTimestampInSeconds = self.tiebreakerActivationTimeout.addTo(Timestamps.now()).toSeconds(); for (uint256 i = 0; i < sealableWithdrawalBlockersCount; ++i) { - (bool isCallSucceed, uint256 resumeSinceTimestampInSeconds) = + (bool isCallSucceed, uint256 sealableResumeSinceTimestampInSeconds) = SealableCalls.callGetResumeSinceTimestamp(self.sealableWithdrawalBlockers.at(i)); - /// @dev If the call failed, mark the sealable as faulty. - /// Otherwise, consider it paused if its pause duration exceeds `tiebreakerActivationTimeout`. - if (!isCallSucceed || tiebreakAllowedTillTimestampInSeconds <= resumeSinceTimestampInSeconds) { + + if (!isCallSucceed || sealableResumeSinceTimestampInSeconds > tiebreakAllowedAfterTimestampInSeconds) { return true; } } diff --git a/test/unit/libraries/Tiebreaker.t.sol b/test/unit/libraries/Tiebreaker.t.sol index 23ccf8e3..333f79b5 100644 --- a/test/unit/libraries/Tiebreaker.t.sol +++ b/test/unit/libraries/Tiebreaker.t.sol @@ -190,16 +190,16 @@ contract TiebreakerTest is UnitTest { // check edge case when resumeSinceTimestamp == block.timestamp + tiebreakerActivationTimeout _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp + tiebreakerActivationTimeout.toSeconds()); - // tiebreaker activation should be allowed + // tiebreaker activation is not allowed + vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); this.external__checkTie(DualGovernanceState.RageQuit, normalOrVetoCooldownExitedAt); - // but when resumeSinceTimestamp == block.timestamp + tiebreakerActivationTimeout - 1 + // but when resumeSinceTimestamp == block.timestamp + tiebreakerActivationTimeout + 1 _mockSealableResumeSinceTimestampResult( - _SEALABLE, block.timestamp + tiebreakerActivationTimeout.toSeconds() - 1 + _SEALABLE, block.timestamp + tiebreakerActivationTimeout.toSeconds() + 1 ); - // tiebreaker activation is not allowed - vm.expectRevert(Tiebreaker.TiebreakNotAllowed.selector); + // tiebreaker activation is allowed this.external__checkTie(DualGovernanceState.RageQuit, normalOrVetoCooldownExitedAt); // if the DG locked for a long period of time, tiebreak becomes allowed in any state From 1d70fa53a9d4f770141af23611c8d27f424d04f9 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Sun, 27 Oct 2024 21:50:59 +0400 Subject: [PATCH 3/3] Forbid addition of paused sealable withdrawal blockers --- contracts/libraries/SealableCalls.sol | 14 ++---- contracts/libraries/Tiebreaker.sol | 7 ++- test/unit/libraries/SealableCalls.t.sol | 63 +++++++++++++++---------- test/unit/libraries/Tiebreaker.t.sol | 12 +++++ 4 files changed, 59 insertions(+), 37 deletions(-) diff --git a/contracts/libraries/SealableCalls.sol b/contracts/libraries/SealableCalls.sol index 04cd1e5c..0a204811 100644 --- a/contracts/libraries/SealableCalls.sol +++ b/contracts/libraries/SealableCalls.sol @@ -3,18 +3,16 @@ pragma solidity 0.8.26; import {ISealable} from "../interfaces/ISealable.sol"; -/// @dev All calls to sealable addresses less than MIN_VALID_SEALABLE_ADDRESS are treated as unsuccessful -/// to prevent potential false positives for current or future precompiled addresses. -address constant MIN_VALID_SEALABLE_ADDRESS = address(1024); - library SealableCalls { /// @notice Attempts to call `ISealable.getResumeSinceTimestamp()` method, returning whether the call succeeded /// and the result of the `ISealable.getResumeSinceTimestamp()` call if it succeeded. /// @dev Performs a static call to the `getResumeSinceTimestamp()` method on the `ISealable` interface. /// Ensures that the function does not revert even if the `sealable` contract does not implement /// the interface, has no code at the address, or returns unexpected data. - /// Calls to addresses less than `MIN_VALID_SEALABLE_ADDRESS` are treated as unsuccessful to prevent - /// potential false positives from current or future precompiled addresses. + /// + /// IMPORTANT: `callGetResumeSinceTimestamp()` may yield false-positive results when called on certain + /// precompiled contract addresses like SHA-256 (address(0x02)) or RIPEMD-160 (address(0x03)). Such cases must + /// be managed outside this function. /// @param sealable The address of the sealable contract to check. /// @return success Indicates whether the call to `getResumeSinceTimestamp()` was successful. /// @return resumeSinceTimestamp The timestamp when the contract is expected to become unpaused. @@ -25,10 +23,6 @@ library SealableCalls { view returns (bool success, uint256 resumeSinceTimestamp) { - if (sealable < MIN_VALID_SEALABLE_ADDRESS) { - return (false, 0); - } - // Low-level call to the `getResumeSinceTimestamp` function on the `sealable` contract (bool isCallSucceed, bytes memory returndata) = sealable.staticcall(abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector)); diff --git a/contracts/libraries/Tiebreaker.sol b/contracts/libraries/Tiebreaker.sol index 87efe991..cbbc6ad6 100644 --- a/contracts/libraries/Tiebreaker.sol +++ b/contracts/libraries/Tiebreaker.sol @@ -67,9 +67,12 @@ library Tiebreaker { if (sealableWithdrawalBlockersCount == maxSealableWithdrawalBlockersCount) { revert SealableWithdrawalBlockersLimitReached(); } - (bool isCallSucceed, /* resumeSinceTimestamp */ ) = + + (bool isCallSucceed, uint256 resumeSinceTimestamp) = SealableCalls.callGetResumeSinceTimestamp(sealableWithdrawalBlocker); - if (!isCallSucceed) { + + // Prevents addition of paused or misbehaving sealables + if (!isCallSucceed || resumeSinceTimestamp >= block.timestamp) { revert InvalidSealable(sealableWithdrawalBlocker); } diff --git a/test/unit/libraries/SealableCalls.t.sol b/test/unit/libraries/SealableCalls.t.sol index 42309459..0a308fac 100644 --- a/test/unit/libraries/SealableCalls.t.sol +++ b/test/unit/libraries/SealableCalls.t.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.26; import {stdError} from "forge-std/StdError.sol"; -import {ISealable, SealableCalls, MIN_VALID_SEALABLE_ADDRESS} from "contracts/libraries/SealableCalls.sol"; +import {ISealable, SealableCalls} from "contracts/libraries/SealableCalls.sol"; import {UnitTest} from "test/utils/unit-test.sol"; import {SealableMock} from "test/mocks/SealableMock.sol"; @@ -84,39 +84,52 @@ contract SealableCallsTest is UnitTest { _assertGetResumeSinceTimestampCallResult({sealable: _SEALABLE, isCallSucceed: false, resumeSinceTimestamp: 0}); } - function test_callGetResumeSinceTimestamp_IsCallSucceedFalse_OnSealableLessThanMinValidSealableAddress() external { - // check addresses (0x00, 0x400) reserved for precompiles. - // Currently Ethereum has only precompiles for 0x01 - 0x09 but new precompiles may be added in the future - for (uint256 i = 0; i < uint160(MIN_VALID_SEALABLE_ADDRESS); ++i) { - address precompile = address(uint160(i)); - _assertGetResumeSinceTimestampCallResult({ - sealable: precompile, - isCallSucceed: false, - resumeSinceTimestamp: 0 - }); - } - - address LAST_INVALID_SEALABLE_ADDRESS = address(uint160(MIN_VALID_SEALABLE_ADDRESS) - 1); - // assuming address MIN_VALID_SEALABLE_ADDRESS and LAST_INVALID_SEALABLE_ADDRESS has - // deployed bytecode which returns some value - _mockSealableResumeSinceTimestampResult(MIN_VALID_SEALABLE_ADDRESS, block.timestamp); - _mockSealableResumeSinceTimestampResult(LAST_INVALID_SEALABLE_ADDRESS, block.timestamp); + // --- + // Test False Positive Results On Precompiles + // --- - // call to the LAST_INVALID_SEALABLE_ADDRESS considered not succeed as made to precompile + function test_callGetResumeSinceTimestamp_IsCallSucceed_FalsePositiveResult_On_SHA256_Precompile() external { _assertGetResumeSinceTimestampCallResult({ - sealable: LAST_INVALID_SEALABLE_ADDRESS, - isCallSucceed: false, - resumeSinceTimestamp: 0 + sealable: address(0x2), + isCallSucceed: true, + resumeSinceTimestamp: 0xc61a1ce4443e07760aea88e1ac096cb3006c1c4284ade7873025b96c2010e1c8 }); + } - // but call to the MIN_VALID_SEALABLE_ADDRESS considered succeed + function test_callGetResumeSinceTimestamp_IsCallSucceed_FalsePositiveResult_On_RIPEMD160_Precompile() external { _assertGetResumeSinceTimestampCallResult({ - sealable: MIN_VALID_SEALABLE_ADDRESS, + sealable: address(0x3), isCallSucceed: true, - resumeSinceTimestamp: block.timestamp + resumeSinceTimestamp: 0x00000000000000000000000075b4744a1c0e92713946840b9adc0cb967652b9c }); } + // --- + // Other Precompiles (Split in 2 parts because of out of gas) + // --- + + function test_callGetResumeSinceTimestamp_IsCallSucceed_CorrectResult_On_Other_Precompiles_Part1() external { + for (uint160 i = 1; i < 8; ++i) { + // Skip SHA-256 and RIPEMD-160 precompiles which lead to false positive results + if (i == 0x2 || i == 0x3) continue; + _assertGetResumeSinceTimestampCallResult({ + sealable: address(i), + isCallSucceed: false, + resumeSinceTimestamp: 0 + }); + } + } + + function test_callGetResumeSinceTimestamp_IsCallSucceed_CorrectResult_On_Other_Precompiles_Part2() external { + for (uint160 i = 8; i < 12; ++i) { + _assertGetResumeSinceTimestampCallResult({ + sealable: address(i), + isCallSucceed: false, + resumeSinceTimestamp: 0 + }); + } + } + // --- // Helper Test Methods // --- diff --git a/test/unit/libraries/Tiebreaker.t.sol b/test/unit/libraries/Tiebreaker.t.sol index 333f79b5..53aa3f2b 100644 --- a/test/unit/libraries/Tiebreaker.t.sol +++ b/test/unit/libraries/Tiebreaker.t.sol @@ -76,6 +76,18 @@ contract TiebreakerTest is UnitTest { vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, _SEALABLE)); this.external__addSealableWithdrawalBlocker(_SEALABLE); + + // revert when sealable is paused for short period + _mockSealableResumeSinceTimestampResult(_SEALABLE, block.timestamp); + vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, _SEALABLE)); + + this.external__addSealableWithdrawalBlocker(_SEALABLE); + + // revert when sealable is paused for long period + _mockSealableResumeSinceTimestampResult(_SEALABLE, type(uint256).max); + vm.expectRevert(abi.encodeWithSelector(Tiebreaker.InvalidSealable.selector, _SEALABLE)); + + this.external__addSealableWithdrawalBlocker(_SEALABLE); } function test_removeSealableWithdrawalBlocker_HappyPath() external {