diff --git a/src/Governance.sol b/src/Governance.sol index f2c8f21..4aa58e7 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -126,7 +126,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own _initiatives[i], MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (1)) ); - emit RegisterInitiative(_initiatives[i], msg.sender, 1, success); + emit RegisterInitiative(_initiatives[i], msg.sender, 1, success ? HookStatus.Succeeded : HookStatus.Failed); } _renounceOwnership(); @@ -511,7 +511,9 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own _initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch)) ); - emit RegisterInitiative(_initiative, msg.sender, currentEpoch, success); + emit RegisterInitiative( + _initiative, msg.sender, currentEpoch, success ? HookStatus.Succeeded : HookStatus.Failed + ); } struct ResetInitiativeData { @@ -803,19 +805,30 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own vars.userState.allocatedOffset = add(vars.userState.allocatedOffset, (vars.deltaOffsetVotes + vars.deltaOffsetVetos)); - // Replaces try / catch | Enforces sufficient gas is passed - bool success = safeCallWithMinGas( - initiative, - MIN_GAS_TO_HOOK, - 0, - abi.encodeCall( - IInitiative.onAfterAllocateLQTY, - (vars.currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState) - ) - ); + HookStatus hookStatus; + + // See https://github.com/liquity/V2-gov/issues/125 + // A malicious initiative could try to dissuade voters from casting vetos by consuming as much gas as + // possible in the `onAfterAllocateLQTY` hook when detecting vetos. + // We deem that the risks of calling into malicous initiatives upon veto allocation far outweigh the + // benefits of notifying benevolent initiatives of vetos. + if (vars.allocation.vetoLQTY == 0) { + // Replaces try / catch | Enforces sufficient gas is passed + hookStatus = safeCallWithMinGas( + initiative, + MIN_GAS_TO_HOOK, + 0, + abi.encodeCall( + IInitiative.onAfterAllocateLQTY, + (vars.currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState) + ) + ) ? HookStatus.Succeeded : HookStatus.Failed; + } else { + hookStatus = HookStatus.NotCalled; + } emit AllocateLQTY( - msg.sender, initiative, vars.deltaLQTYVotes, vars.deltaLQTYVetos, vars.currentEpoch, success + msg.sender, initiative, vars.deltaLQTYVotes, vars.deltaLQTYVetos, vars.currentEpoch, hookStatus ); } @@ -861,7 +874,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own _initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch)) ); - emit UnregisterInitiative(_initiative, currentEpoch, success); + emit UnregisterInitiative(_initiative, currentEpoch, success ? HookStatus.Succeeded : HookStatus.Failed); } /// @inheritdoc IGovernance @@ -905,7 +918,9 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claimableAmount)) ); - emit ClaimForInitiative(_initiative, claimableAmount, votesSnapshot_.forEpoch, success); + emit ClaimForInitiative( + _initiative, claimableAmount, votesSnapshot_.forEpoch, success ? HookStatus.Succeeded : HookStatus.Failed + ); return claimableAmount; } diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 1169178..167aff3 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -10,6 +10,12 @@ import {PermitParams} from "../utils/Types.sol"; uint256 constant UNREGISTERED_INITIATIVE = type(uint256).max; interface IGovernance { + enum HookStatus { + Failed, + Succeeded, + NotCalled + } + /// @notice Emitted when a user deposits LQTY /// @param user The account depositing LQTY /// @param rewardRecipient The account receiving the LUSD/ETH rewards earned from staking in V1, if claimed @@ -51,8 +57,8 @@ interface IGovernance { event SnapshotVotes(uint256 votes, uint256 forEpoch, uint256 boldAccrued); event SnapshotVotesForInitiative(address indexed initiative, uint256 votes, uint256 vetos, uint256 forEpoch); - event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, bool hookSuccess); - event UnregisterInitiative(address initiative, uint256 atEpoch, bool hookSuccess); + event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, HookStatus hookStatus); + event UnregisterInitiative(address initiative, uint256 atEpoch, HookStatus hookStatus); event AllocateLQTY( address indexed user, @@ -60,9 +66,9 @@ interface IGovernance { int256 deltaVoteLQTY, int256 deltaVetoLQTY, uint256 atEpoch, - bool hookSuccess + HookStatus hookStatus ); - event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, bool hookSuccess); + event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, HookStatus hookStatus); struct Configuration { uint256 registrationFee; diff --git a/test/InitiativeHooks.t.sol b/test/InitiativeHooks.t.sol new file mode 100644 index 0000000..e5ae2c0 --- /dev/null +++ b/test/InitiativeHooks.t.sol @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import {IGovernance} from "../src/interfaces/IGovernance.sol"; +import {IInitiative} from "../src/interfaces/IInitiative.sol"; +import {Governance} from "../src/Governance.sol"; +import {MockERC20Tester} from "./mocks/MockERC20Tester.sol"; +import {MockStakingV1} from "./mocks/MockStakingV1.sol"; +import {MockStakingV1Deployer} from "./mocks/MockStakingV1Deployer.sol"; + +contract MockInitiative is IInitiative { + struct OnAfterAllocateLQTYParams { + uint256 currentEpoch; + address user; + IGovernance.UserState userState; + IGovernance.Allocation allocation; + IGovernance.InitiativeState initiativeStat; + } + + OnAfterAllocateLQTYParams[] public onAfterAllocateLQTYCalls; + + function numOnAfterAllocateLQTYCalls() external view returns (uint256) { + return onAfterAllocateLQTYCalls.length; + } + + function onAfterAllocateLQTY( + uint256 _currentEpoch, + address _user, + IGovernance.UserState calldata _userState, + IGovernance.Allocation calldata _allocation, + IGovernance.InitiativeState calldata _initiativeState + ) external override { + onAfterAllocateLQTYCalls.push( + OnAfterAllocateLQTYParams(_currentEpoch, _user, _userState, _allocation, _initiativeState) + ); + } + + function onRegisterInitiative(uint256) external override {} + function onUnregisterInitiative(uint256) external override {} + function onClaimForInitiative(uint256, uint256) external override {} +} + +contract InitiativeHooksTest is MockStakingV1Deployer { + uint32 constant START_TIME = 1732873631; + uint32 constant EPOCH_DURATION = 7 days; + uint32 constant EPOCH_VOTING_CUTOFF = 6 days; + + IGovernance.Configuration config = IGovernance.Configuration({ + registrationFee: 0, + registrationThresholdFactor: 0, + unregistrationThresholdFactor: 4 ether, + unregistrationAfterEpochs: 4, + votingThresholdFactor: 0, + minClaim: 0, + minAccrual: 0, + epochStart: START_TIME - EPOCH_DURATION, + epochDuration: EPOCH_DURATION, + epochVotingCutoff: EPOCH_VOTING_CUTOFF + }); + + MockStakingV1 stakingV1; + MockERC20Tester lqty; + MockERC20Tester lusd; + MockERC20Tester bold; + Governance governance; + MockInitiative initiative; + address[] noInitiatives; // left empty + address[] initiatives; + int256[] votes; + int256[] vetos; + address voter; + + function setUp() external { + vm.warp(START_TIME); + + (stakingV1, lqty, lusd) = deployMockStakingV1(); + + bold = new MockERC20Tester("BOLD Stablecoin", "BOLD"); + vm.label(address(bold), "BOLD"); + + governance = new Governance({ + _lqty: address(lqty), + _lusd: address(lusd), + _stakingV1: address(stakingV1), + _bold: address(bold), + _config: config, + _owner: address(this), + _initiatives: new address[](0) + }); + + initiative = new MockInitiative(); + initiatives.push(address(initiative)); + governance.registerInitialInitiatives(initiatives); + + voter = makeAddr("voter"); + lqty.mint(voter, 1 ether); + + vm.startPrank(voter); + lqty.approve(governance.deriveUserProxyAddress(voter), type(uint256).max); + governance.depositLQTY(1 ether); + vm.stopPrank(); + + votes.push(); + vetos.push(); + } + + function test_OnAfterAllocateLQTY_IsCalled_WhenCastingVotes() external { + vm.startPrank(voter); + votes[0] = 123; + governance.allocateLQTY(noInitiatives, initiatives, votes, vetos); + vm.stopPrank(); + + assertEq(initiative.numOnAfterAllocateLQTYCalls(), 1, "onAfterAllocateLQTY should have been called once"); + (,,, IGovernance.Allocation memory allocation,) = initiative.onAfterAllocateLQTYCalls(0); + assertEq(allocation.voteLQTY, 123, "wrong voteLQTY 1"); + + vm.startPrank(voter); + votes[0] = 456; + governance.allocateLQTY(initiatives, initiatives, votes, vetos); + vm.stopPrank(); + + assertEq(initiative.numOnAfterAllocateLQTYCalls(), 3, "onAfterAllocateLQTY should have been called twice more"); + (,,, allocation,) = initiative.onAfterAllocateLQTYCalls(1); + assertEq(allocation.voteLQTY, 0, "wrong voteLQTY 2"); + (,,, allocation,) = initiative.onAfterAllocateLQTYCalls(2); + assertEq(allocation.voteLQTY, 456, "wrong voteLQTY 3"); + } + + function test_OnAfterAllocateLQTY_IsNotCalled_WhenCastingVetos() external { + vm.startPrank(voter); + vetos[0] = 123; + governance.allocateLQTY(noInitiatives, initiatives, votes, vetos); + vm.stopPrank(); + + assertEq(initiative.numOnAfterAllocateLQTYCalls(), 0, "onAfterAllocateLQTY should not have been called once"); + } + + function test_OnAfterAllocateLQTY_IsCalledOnceWithZeroVotes_WhenCastingVetosAfterHavingCastVotes() external { + vm.startPrank(voter); + votes[0] = 123; + governance.allocateLQTY(noInitiatives, initiatives, votes, vetos); + vm.stopPrank(); + + assertEq(initiative.numOnAfterAllocateLQTYCalls(), 1, "onAfterAllocateLQTY should have been called once"); + + vm.startPrank(voter); + votes[0] = 0; + vetos[0] = 456; + governance.allocateLQTY(initiatives, initiatives, votes, vetos); + vm.stopPrank(); + + assertEq(initiative.numOnAfterAllocateLQTYCalls(), 2, "onAfterAllocateLQTY should have been called once more"); + (,,, IGovernance.Allocation memory allocation,) = initiative.onAfterAllocateLQTYCalls(1); + assertEq(allocation.voteLQTY, 0, "wrong voteLQTY"); + } +}