Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit fix: Discrepancies Between Documentation and Implementation #171

Merged
merged 3 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion contracts/interfaces/ISealable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
83 changes: 23 additions & 60 deletions contracts/libraries/SealableCalls.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,71 +3,34 @@ 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.
/// @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.
///
/// @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 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
/// 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.
/// 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;
}
}
// Low-level call to the `getResumeSinceTimestamp` function on the `sealable` contract
(bool isCallSucceed, bytes memory returndata) =
sealable.staticcall(abi.encodeWithSelector(ISealable.getResumeSinceTimestamp.selector));

/// @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;
// 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));
}
}
}
44 changes: 31 additions & 13 deletions contracts/libraries/Tiebreaker.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -68,8 +67,12 @@ library Tiebreaker {
if (sealableWithdrawalBlockersCount == maxSealableWithdrawalBlockersCount) {
revert SealableWithdrawalBlockersLimitReached();
}
(bool isCallSucceed, /* lowLevelError */, /* isPaused */ ) = ISealable(sealableWithdrawalBlocker).callIsPaused();
if (!isCallSucceed) {

(bool isCallSucceed, uint256 resumeSinceTimestamp) =
SealableCalls.callGetResumeSinceTimestamp(sealableWithdrawalBlocker);

// Prevents addition of paused or misbehaving sealables
if (!isCallSucceed || resumeSinceTimestamp >= block.timestamp) {
revert InvalidSealable(sealableWithdrawalBlocker);
}

Expand Down Expand Up @@ -172,19 +175,34 @@ 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 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`
/// or is functioning incorrectly, false otherwise.
function isSomeSealableWithdrawalBlockerPausedForLongTermOrFaulty(Context storage self)
internal
view
returns (bool)
{
uint256 sealableWithdrawalBlockersCount = self.sealableWithdrawalBlockers.length();

/// @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, /* lowLevelError */, bool isPaused) =
ISealable(self.sealableWithdrawalBlockers.at(i)).callIsPaused();
(bool isCallSucceed, uint256 sealableResumeSinceTimestampInSeconds) =
SealableCalls.callGetResumeSinceTimestamp(self.sealableWithdrawalBlockers.at(i));

if (isPaused || !isCallSucceed) return true;
if (!isCallSucceed || sealableResumeSinceTimestampInSeconds > tiebreakAllowedAfterTimestampInSeconds) {
return true;
}
}
return false;
}
Expand Down
37 changes: 0 additions & 37 deletions test/mocks/GateSealMock.sol

This file was deleted.

20 changes: 10 additions & 10 deletions test/mocks/SealableMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
}
}
12 changes: 9 additions & 3 deletions test/unit/DualGovernance.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading