Merge pull request #171 from lidofinance/fix/sync-code-and-spec

Audit fix: Discrepancies Between Documentation and Implementation

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);
 }

contracts/libraries/SealableCalls.sol

@@ -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));
         }
     }
 }

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";
 ///     This library includes functions for managing a standalone tiebreaker committee, which can resolve such deadlocks
 ///     by executing pending proposals or unpausing protocol contracts, but only under specific deadlock conditions.
 library Tiebreaker {
-    using SealableCalls for ISealable;
     using EnumerableSet for EnumerableSet.AddressSet;
 
     // ---
@@ -82,8 +81,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);
         }
 
@@ -182,19 +185,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;
     }

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;
     }
 }

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);