From 16631755e1bb535e0ee146a4636f82c295511c56 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 28 Aug 2024 04:47:39 +0400 Subject: [PATCH 1/2] Fix the overflow of the rage quit round --- .../libraries/DualGovernanceStateMachine.sol | 14 +++- test/mocks/EscrowMock.sol | 43 ++++++++++ .../DualGovernanceStateMachine.t.sol | 80 +++++++++++++++++++ 3 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 test/mocks/EscrowMock.sol create mode 100644 test/unit/libraries/DualGovernanceStateMachine.t.sol diff --git a/contracts/libraries/DualGovernanceStateMachine.sol b/contracts/libraries/DualGovernanceStateMachine.sol index 59a9c810..d783d55f 100644 --- a/contracts/libraries/DualGovernanceStateMachine.sol +++ b/contracts/libraries/DualGovernanceStateMachine.sol @@ -64,6 +64,8 @@ library DualGovernanceStateMachine { event NewSignallingEscrowDeployed(IEscrow indexed escrow); event DualGovernanceStateChanged(State from, State to, Context state); + uint256 internal constant MAX_RAGE_QUIT_ROUND = type(uint8).max; + function initialize( Context storage self, DualGovernanceConfig.Context memory config, @@ -108,10 +110,16 @@ library DualGovernanceStateMachine { } } else if (newState == State.RageQuit) { IEscrow signallingEscrow = self.signallingEscrow; - uint256 rageQuitRound = Math.min(self.rageQuitRound + 1, type(uint8).max); - self.rageQuitRound = uint8(rageQuitRound); + + uint256 currentRageQuitRound = self.rageQuitRound; + + /// @dev Limits the maximum value of the rage quit round to prevent failures due to arithmetic overflow + /// if the number of consecutive rage quits reaches MAX_RAGE_QUIT_ROUND. + uint256 newRageQuitRound = Math.min(currentRageQuitRound + 1, MAX_RAGE_QUIT_ROUND); + self.rageQuitRound = uint8(newRageQuitRound); + signallingEscrow.startRageQuit( - config.rageQuitExtensionDelay, config.calcRageQuitWithdrawalsTimelock(rageQuitRound) + config.rageQuitExtensionDelay, config.calcRageQuitWithdrawalsTimelock(newRageQuitRound) ); self.rageQuitEscrow = signallingEscrow; _deployNewSignallingEscrow(self, escrowMasterCopy, config.minAssetsLockDuration); diff --git a/test/mocks/EscrowMock.sol b/test/mocks/EscrowMock.sol new file mode 100644 index 00000000..fe3e0798 --- /dev/null +++ b/test/mocks/EscrowMock.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.26; + +import {Duration} from "contracts/types/Duration.sol"; +import {PercentD16} from "contracts/types/PercentD16.sol"; + +import {IEscrow} from "contracts/interfaces/IEscrow.sol"; + +contract EscrowMock is IEscrow { + event __RageQuitStarted(Duration rageQuitExtraTimelock, Duration rageQuitWithdrawalsTimelock); + + Duration public __minAssetsLockDuration; + PercentD16 public __rageQuitSupport; + bool public __isRageQuitFinalized; + + function __setRageQuitSupport(PercentD16 newRageQuitSupport) external { + __rageQuitSupport = newRageQuitSupport; + } + + function __setIsRageQuitFinalized(bool newIsRageQuitFinalized) external { + __isRageQuitFinalized = newIsRageQuitFinalized; + } + + function initialize(Duration minAssetsLockDuration) external { + __minAssetsLockDuration = minAssetsLockDuration; + } + + function startRageQuit(Duration rageQuitExtraTimelock, Duration rageQuitWithdrawalsTimelock) external { + emit __RageQuitStarted(rageQuitExtraTimelock, rageQuitWithdrawalsTimelock); + } + + function isRageQuitFinalized() external view returns (bool) { + return __isRageQuitFinalized; + } + + function getRageQuitSupport() external view returns (PercentD16 rageQuitSupport) { + return __rageQuitSupport; + } + + function setMinAssetsLockDuration(Duration newMinAssetsLockDuration) external { + __minAssetsLockDuration = newMinAssetsLockDuration; + } +} diff --git a/test/unit/libraries/DualGovernanceStateMachine.t.sol b/test/unit/libraries/DualGovernanceStateMachine.t.sol new file mode 100644 index 00000000..8aafa5c8 --- /dev/null +++ b/test/unit/libraries/DualGovernanceStateMachine.t.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.26; + +import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; + +import {Durations} from "contracts/types/Duration.sol"; +import {PercentsD16} from "contracts/types/PercentD16.sol"; + +import {DualGovernanceStateMachine, State} from "contracts/libraries/DualGovernanceStateMachine.sol"; +import {DualGovernanceConfig, ImmutableDualGovernanceConfigProvider} from "contracts/DualGovernanceConfigProvider.sol"; + +import {UnitTest} from "test/utils/unit-test.sol"; +import {EscrowMock} from "test/mocks/EscrowMock.sol"; + +contract DualGovernanceStateMachineUnitTests is UnitTest { + using DualGovernanceStateMachine for DualGovernanceStateMachine.Context; + + address private immutable _ESCROW_MASTER_COPY = address(new EscrowMock()); + ImmutableDualGovernanceConfigProvider internal immutable _CONFIG_PROVIDER = new ImmutableDualGovernanceConfigProvider( + DualGovernanceConfig.Context({ + firstSealRageQuitSupport: PercentsD16.fromBasisPoints(3_00), // 3% + secondSealRageQuitSupport: PercentsD16.fromBasisPoints(15_00), // 15% + // + minAssetsLockDuration: Durations.from(5 hours), + dynamicTimelockMinDuration: Durations.from(3 days), + dynamicTimelockMaxDuration: Durations.from(30 days), + // + vetoSignallingMinActiveDuration: Durations.from(5 hours), + vetoSignallingDeactivationMaxDuration: Durations.from(5 days), + vetoCooldownDuration: Durations.from(4 days), + // + rageQuitExtensionDelay: Durations.from(7 days), + rageQuitEthWithdrawalsMinTimelock: Durations.from(60 days), + rageQuitEthWithdrawalsTimelockGrowthStartSeqNumber: 2, + rageQuitEthWithdrawalsTimelockGrowthCoeffs: [uint256(0), 0, 0] + }) + ); + + DualGovernanceStateMachine.Context private _stateMachine; + + function setUp() external { + _stateMachine.initialize(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + } + + function test_activateNextState_HappyPath_MaxRageQuitsRound() external { + assertEq(_stateMachine.state, State.Normal); + + for (uint256 i = 0; i < 2 * DualGovernanceStateMachine.MAX_RAGE_QUIT_ROUND; ++i) { + address signallingEscrow = address(_stateMachine.signallingEscrow); + EscrowMock(signallingEscrow).__setRageQuitSupport( + _CONFIG_PROVIDER.SECOND_SEAL_RAGE_QUIT_SUPPORT() + PercentsD16.fromBasisPoints(1_00) + ); + assertTrue( + _stateMachine.signallingEscrow.getRageQuitSupport() > _CONFIG_PROVIDER.SECOND_SEAL_RAGE_QUIT_SUPPORT() + ); + assertEq(_stateMachine.rageQuitRound, Math.min(i, DualGovernanceStateMachine.MAX_RAGE_QUIT_ROUND)); + + // wait here the full duration of the veto cooldown to make sure it's over from the previous iteration + _wait(_CONFIG_PROVIDER.VETO_COOLDOWN_DURATION().plusSeconds(1)); + + _stateMachine.activateNextState(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + assertEq(_stateMachine.state, State.VetoSignalling); + + _wait(_CONFIG_PROVIDER.DYNAMIC_TIMELOCK_MAX_DURATION().plusSeconds(1)); + _stateMachine.activateNextState(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + + assertEq(_stateMachine.state, State.RageQuit); + assertEq(_stateMachine.rageQuitRound, Math.min(i + 1, DualGovernanceStateMachine.MAX_RAGE_QUIT_ROUND)); + + EscrowMock(signallingEscrow).__setIsRageQuitFinalized(true); + _stateMachine.activateNextState(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + assertEq(_stateMachine.state, State.VetoCooldown); + } + + // after the sequential rage quits chain is broken, the rage quit resets to 0 + _wait(_CONFIG_PROVIDER.VETO_COOLDOWN_DURATION().plusSeconds(1)); + _stateMachine.activateNextState(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + assertEq(_stateMachine.state, State.Normal); + } +} From 51ad0efe33a2fee1f0239295c5ed35d5ec0d9b5c Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 28 Aug 2024 17:03:57 +0400 Subject: [PATCH 2/2] Add check for rageQuitRound into the unit test --- test/unit/libraries/DualGovernanceStateMachine.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/libraries/DualGovernanceStateMachine.t.sol b/test/unit/libraries/DualGovernanceStateMachine.t.sol index 8aafa5c8..3e5d6cdf 100644 --- a/test/unit/libraries/DualGovernanceStateMachine.t.sol +++ b/test/unit/libraries/DualGovernanceStateMachine.t.sol @@ -75,6 +75,8 @@ contract DualGovernanceStateMachineUnitTests is UnitTest { // after the sequential rage quits chain is broken, the rage quit resets to 0 _wait(_CONFIG_PROVIDER.VETO_COOLDOWN_DURATION().plusSeconds(1)); _stateMachine.activateNextState(_CONFIG_PROVIDER.getDualGovernanceConfig(), _ESCROW_MASTER_COPY); + + assertEq(_stateMachine.rageQuitRound, 0); assertEq(_stateMachine.state, State.Normal); } }