Skip to content

Commit 1c379b5

Browse files
bingenbingen
authored
Merge pull request #126 from liquity/no-veto-hook
feat: don't call `onAfterAllocateLQTY()` on vetos
2 parents ac09a1a + 4258eb3 commit 1c379b5

File tree

3 files changed

+196
-19
lines changed

3 files changed

+196
-19
lines changed

‎src/Governance.sol

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own
126126
_initiatives[i], MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (1))
127127
);
128128

129-
emit RegisterInitiative(_initiatives[i], msg.sender, 1, success);
129+
emit RegisterInitiative(_initiatives[i], msg.sender, 1, success ? HookStatus.Succeeded : HookStatus.Failed);
130130
}
131131

132132
_renounceOwnership();
@@ -511,7 +511,9 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own
511511
_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch))
512512
);
513513

514-
emit RegisterInitiative(_initiative, msg.sender, currentEpoch, success);
514+
emit RegisterInitiative(
515+
_initiative, msg.sender, currentEpoch, success ? HookStatus.Succeeded : HookStatus.Failed
516+
);
515517
}
516518

517519
struct ResetInitiativeData {
@@ -803,19 +805,30 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own
803805
vars.userState.allocatedOffset =
804806
add(vars.userState.allocatedOffset, (vars.deltaOffsetVotes + vars.deltaOffsetVetos));
805807

806-
// Replaces try / catch | Enforces sufficient gas is passed
807-
bool success = safeCallWithMinGas(
808-
initiative,
809-
MIN_GAS_TO_HOOK,
810-
0,
811-
abi.encodeCall(
812-
IInitiative.onAfterAllocateLQTY,
813-
(vars.currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState)
814-
)
815-
);
808+
HookStatus hookStatus;
809+
810+
// See https://github.com/liquity/V2-gov/issues/125
811+
// A malicious initiative could try to dissuade voters from casting vetos by consuming as much gas as
812+
// possible in the `onAfterAllocateLQTY` hook when detecting vetos.
813+
// We deem that the risks of calling into malicous initiatives upon veto allocation far outweigh the
814+
// benefits of notifying benevolent initiatives of vetos.
815+
if (vars.allocation.vetoLQTY == 0) {
816+
// Replaces try / catch | Enforces sufficient gas is passed
817+
hookStatus = safeCallWithMinGas(
818+
initiative,
819+
MIN_GAS_TO_HOOK,
820+
0,
821+
abi.encodeCall(
822+
IInitiative.onAfterAllocateLQTY,
823+
(vars.currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState)
824+
)
825+
) ? HookStatus.Succeeded : HookStatus.Failed;
826+
} else {
827+
hookStatus = HookStatus.NotCalled;
828+
}
816829

817830
emit AllocateLQTY(
818-
msg.sender, initiative, vars.deltaLQTYVotes, vars.deltaLQTYVetos, vars.currentEpoch, success
831+
msg.sender, initiative, vars.deltaLQTYVotes, vars.deltaLQTYVetos, vars.currentEpoch, hookStatus
819832
);
820833
}
821834

@@ -861,7 +874,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own
861874
_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch))
862875
);
863876

864-
emit UnregisterInitiative(_initiative, currentEpoch, success);
877+
emit UnregisterInitiative(_initiative, currentEpoch, success ? HookStatus.Succeeded : HookStatus.Failed);
865878
}
866879

867880
/// @inheritdoc IGovernance
@@ -905,7 +918,9 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own
905918
abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claimableAmount))
906919
);
907920

908-
emit ClaimForInitiative(_initiative, claimableAmount, votesSnapshot_.forEpoch, success);
921+
emit ClaimForInitiative(
922+
_initiative, claimableAmount, votesSnapshot_.forEpoch, success ? HookStatus.Succeeded : HookStatus.Failed
923+
);
909924

910925
return claimableAmount;
911926
}

‎src/interfaces/IGovernance.sol

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,12 @@ import {PermitParams} from "../utils/Types.sol";
1010
uint256 constant UNREGISTERED_INITIATIVE = type(uint256).max;
1111

1212
interface IGovernance {
13+
enum HookStatus {
14+
Failed,
15+
Succeeded,
16+
NotCalled
17+
}
18+
1319
/// @notice Emitted when a user deposits LQTY
1420
/// @param user The account depositing LQTY
1521
/// @param rewardRecipient The account receiving the LUSD/ETH rewards earned from staking in V1, if claimed
@@ -51,18 +57,18 @@ interface IGovernance {
5157
event SnapshotVotes(uint256 votes, uint256 forEpoch, uint256 boldAccrued);
5258
event SnapshotVotesForInitiative(address indexed initiative, uint256 votes, uint256 vetos, uint256 forEpoch);
5359

54-
event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, bool hookSuccess);
55-
event UnregisterInitiative(address initiative, uint256 atEpoch, bool hookSuccess);
60+
event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, HookStatus hookStatus);
61+
event UnregisterInitiative(address initiative, uint256 atEpoch, HookStatus hookStatus);
5662

5763
event AllocateLQTY(
5864
address indexed user,
5965
address indexed initiative,
6066
int256 deltaVoteLQTY,
6167
int256 deltaVetoLQTY,
6268
uint256 atEpoch,
63-
bool hookSuccess
69+
HookStatus hookStatus
6470
);
65-
event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, bool hookSuccess);
71+
event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, HookStatus hookStatus);
6672

6773
struct Configuration {
6874
uint256 registrationFee;

‎test/InitiativeHooks.t.sol

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
// SPDX-License-Identifier: MIT
2+
pragma solidity ^0.8.24;
3+
4+
import {IGovernance} from "../src/interfaces/IGovernance.sol";
5+
import {IInitiative} from "../src/interfaces/IInitiative.sol";
6+
import {Governance} from "../src/Governance.sol";
7+
import {MockERC20Tester} from "./mocks/MockERC20Tester.sol";
8+
import {MockStakingV1} from "./mocks/MockStakingV1.sol";
9+
import {MockStakingV1Deployer} from "./mocks/MockStakingV1Deployer.sol";
10+
11+
contract MockInitiative is IInitiative {
12+
struct OnAfterAllocateLQTYParams {
13+
uint256 currentEpoch;
14+
address user;
15+
IGovernance.UserState userState;
16+
IGovernance.Allocation allocation;
17+
IGovernance.InitiativeState initiativeStat;
18+
}
19+
20+
OnAfterAllocateLQTYParams[] public onAfterAllocateLQTYCalls;
21+
22+
function numOnAfterAllocateLQTYCalls() external view returns (uint256) {
23+
return onAfterAllocateLQTYCalls.length;
24+
}
25+
26+
function onAfterAllocateLQTY(
27+
uint256 _currentEpoch,
28+
address _user,
29+
IGovernance.UserState calldata _userState,
30+
IGovernance.Allocation calldata _allocation,
31+
IGovernance.InitiativeState calldata _initiativeState
32+
) external override {
33+
onAfterAllocateLQTYCalls.push(
34+
OnAfterAllocateLQTYParams(_currentEpoch, _user, _userState, _allocation, _initiativeState)
35+
);
36+
}
37+
38+
function onRegisterInitiative(uint256) external override {}
39+
function onUnregisterInitiative(uint256) external override {}
40+
function onClaimForInitiative(uint256, uint256) external override {}
41+
}
42+
43+
contract InitiativeHooksTest is MockStakingV1Deployer {
44+
uint32 constant START_TIME = 1732873631;
45+
uint32 constant EPOCH_DURATION = 7 days;
46+
uint32 constant EPOCH_VOTING_CUTOFF = 6 days;
47+
48+
IGovernance.Configuration config = IGovernance.Configuration({
49+
registrationFee: 0,
50+
registrationThresholdFactor: 0,
51+
unregistrationThresholdFactor: 4 ether,
52+
unregistrationAfterEpochs: 4,
53+
votingThresholdFactor: 0,
54+
minClaim: 0,
55+
minAccrual: 0,
56+
epochStart: START_TIME - EPOCH_DURATION,
57+
epochDuration: EPOCH_DURATION,
58+
epochVotingCutoff: EPOCH_VOTING_CUTOFF
59+
});
60+
61+
MockStakingV1 stakingV1;
62+
MockERC20Tester lqty;
63+
MockERC20Tester lusd;
64+
MockERC20Tester bold;
65+
Governance governance;
66+
MockInitiative initiative;
67+
address[] noInitiatives; // left empty
68+
address[] initiatives;
69+
int256[] votes;
70+
int256[] vetos;
71+
address voter;
72+
73+
function setUp() external {
74+
vm.warp(START_TIME);
75+
76+
(stakingV1, lqty, lusd) = deployMockStakingV1();
77+
78+
bold = new MockERC20Tester("BOLD Stablecoin", "BOLD");
79+
vm.label(address(bold), "BOLD");
80+
81+
governance = new Governance({
82+
_lqty: address(lqty),
83+
_lusd: address(lusd),
84+
_stakingV1: address(stakingV1),
85+
_bold: address(bold),
86+
_config: config,
87+
_owner: address(this),
88+
_initiatives: new address[](0)
89+
});
90+
91+
initiative = new MockInitiative();
92+
initiatives.push(address(initiative));
93+
governance.registerInitialInitiatives(initiatives);
94+
95+
voter = makeAddr("voter");
96+
lqty.mint(voter, 1 ether);
97+
98+
vm.startPrank(voter);
99+
lqty.approve(governance.deriveUserProxyAddress(voter), type(uint256).max);
100+
governance.depositLQTY(1 ether);
101+
vm.stopPrank();
102+
103+
votes.push();
104+
vetos.push();
105+
}
106+
107+
function test_OnAfterAllocateLQTY_IsCalled_WhenCastingVotes() external {
108+
vm.startPrank(voter);
109+
votes[0] = 123;
110+
governance.allocateLQTY(noInitiatives, initiatives, votes, vetos);
111+
vm.stopPrank();
112+
113+
assertEq(initiative.numOnAfterAllocateLQTYCalls(), 1, "onAfterAllocateLQTY should have been called once");
114+
(,,, IGovernance.Allocation memory allocation,) = initiative.onAfterAllocateLQTYCalls(0);
115+
assertEq(allocation.voteLQTY, 123, "wrong voteLQTY 1");
116+
117+
vm.startPrank(voter);
118+
votes[0] = 456;
119+
governance.allocateLQTY(initiatives, initiatives, votes, vetos);
120+
vm.stopPrank();
121+
122+
assertEq(initiative.numOnAfterAllocateLQTYCalls(), 3, "onAfterAllocateLQTY should have been called twice more");
123+
(,,, allocation,) = initiative.onAfterAllocateLQTYCalls(1);
124+
assertEq(allocation.voteLQTY, 0, "wrong voteLQTY 2");
125+
(,,, allocation,) = initiative.onAfterAllocateLQTYCalls(2);
126+
assertEq(allocation.voteLQTY, 456, "wrong voteLQTY 3");
127+
}
128+
129+
function test_OnAfterAllocateLQTY_IsNotCalled_WhenCastingVetos() external {
130+
vm.startPrank(voter);
131+
vetos[0] = 123;
132+
governance.allocateLQTY(noInitiatives, initiatives, votes, vetos);
133+
vm.stopPrank();
134+
135+
assertEq(initiative.numOnAfterAllocateLQTYCalls(), 0, "onAfterAllocateLQTY should not have been called once");
136+
}
137+
138+
function test_OnAfterAllocateLQTY_IsCalledOnceWithZeroVotes_WhenCastingVetosAfterHavingCastVotes() external {
139+
vm.startPrank(voter);
140+
votes[0] = 123;
141+
governance.allocateLQTY(noInitiatives, initiatives, votes, vetos);
142+
vm.stopPrank();
143+
144+
assertEq(initiative.numOnAfterAllocateLQTYCalls(), 1, "onAfterAllocateLQTY should have been called once");
145+
146+
vm.startPrank(voter);
147+
votes[0] = 0;
148+
vetos[0] = 456;
149+
governance.allocateLQTY(initiatives, initiatives, votes, vetos);
150+
vm.stopPrank();
151+
152+
assertEq(initiative.numOnAfterAllocateLQTYCalls(), 2, "onAfterAllocateLQTY should have been called once more");
153+
(,,, IGovernance.Allocation memory allocation,) = initiative.onAfterAllocateLQTYCalls(1);
154+
assertEq(allocation.voteLQTY, 0, "wrong voteLQTY");
155+
}
156+
}

0 commit comments

Comments
 (0)