From bdf483225f96c31c3c5ade7488452c9c16855bbc Mon Sep 17 00:00:00 2001 From: gallo Date: Mon, 7 Oct 2024 09:52:40 +0200 Subject: [PATCH 1/9] chore: notes for V4 fixes --- src/UniV4Donations.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/UniV4Donations.sol b/src/UniV4Donations.sol index 654aa01..4e223bf 100644 --- a/src/UniV4Donations.sol +++ b/src/UniV4Donations.sol @@ -85,6 +85,8 @@ contract UniV4Donations is BribeInitiative, BaseHook { } function _donateToPool() internal returns (uint256) { + /// @audit TODO: Need to use storage value here I think + /// TODO: Test and fix release speed, which looks off Vesting memory _vesting = _restartVesting(uint240(governance.claimForInitiative(address(this)))); uint256 amount = (_vesting.amount * (block.timestamp - vestingEpochStart()) / VESTING_EPOCH_DURATION) - _vesting.released; From 175d782d4b1e984c6d4b83e1b8dea8645e051910 Mon Sep 17 00:00:00 2001 From: gallo Date: Mon, 7 Oct 2024 10:09:45 +0200 Subject: [PATCH 2/9] feat: low gas protected --- src/Governance.sol | 21 ++++++++++++------ src/utils/SafeCallMinGas.sol | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 src/utils/SafeCallMinGas.sol diff --git a/src/Governance.sol b/src/Governance.sol index 2f361b3..d60caf0 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -15,6 +15,7 @@ import {UserProxyFactory} from "./UserProxyFactory.sol"; import {add, max} from "./utils/Math.sol"; import {Multicall} from "./utils/Multicall.sol"; import {WAD, PermitParams} from "./utils/Types.sol"; +import {safeCallWithMinGas} from "./utils/SafeCallMinGas.sol"; /// @title Governance: Modular Initiative based Governance contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance { @@ -323,7 +324,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance emit RegisterInitiative(_initiative, msg.sender, currentEpoch); - try IInitiative(_initiative).onRegisterInitiative(currentEpoch) {} catch {} + // try IInitiative(_initiative).onRegisterInitiative(currentEpoch) {} catch {} + // Replaces try / catch | Enforces sufficient gas is passed + safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch))); } /// @inheritdoc IGovernance @@ -369,7 +372,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance emit UnregisterInitiative(_initiative, currentEpoch); - try IInitiative(_initiative).onUnregisterInitiative(currentEpoch) {} catch {} + // try IInitiative(_initiative).onUnregisterInitiative(currentEpoch) {} catch {} + // Replaces try / catch | Enforces sufficient gas is passed + safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch))); } /// @inheritdoc IGovernance @@ -477,9 +482,11 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch); - try IInitiative(initiative).onAfterAllocateLQTY( - currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY - ) {} catch {} + // try IInitiative(initiative).onAfterAllocateLQTY( + // currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY + // ) {} catch {} + // Replaces try / catch | Enforces sufficient gas is passed + safeCallWithMinGas(initiative, 350_000, 0, abi.encodeCall(IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY))); } require( @@ -509,7 +516,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance emit ClaimForInitiative(_initiative, claim, votesSnapshot_.forEpoch); - try IInitiative(_initiative).onClaimForInitiative(votesSnapshot_.forEpoch, claim) {} catch {} + // try IInitiative(_initiative).onClaimForInitiative(votesSnapshot_.forEpoch, claim) {} catch {} + // Replaces try / catch | Enforces sufficient gas is passed + safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claim))); return claim; } diff --git a/src/utils/SafeCallMinGas.sol b/src/utils/SafeCallMinGas.sol new file mode 100644 index 0000000..186b90a --- /dev/null +++ b/src/utils/SafeCallMinGas.sol @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @notice Given the gas requirement, ensures that the current context has sufficient gas to perform a call + a fixed buffer +/// @dev Credits: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/libraries/SafeCall.sol#L100-L107 +function hasMinGas(uint256 _minGas, uint256 _reservedGas) view returns (bool) { + bool _hasMinGas; + assembly { + // Equation: gas × 63 ≥ minGas × 64 + 63(40_000 + reservedGas) + _hasMinGas := iszero(lt(mul(gas(), 63), add(mul(_minGas, 64), mul(add(40000, _reservedGas), 63)))) + } + return _hasMinGas; +} + +/// @dev Performs a call ignoring the recipient existing or not, passing the exact gas value, ignoring any return value +function safeCallWithMinGas( + address _target, + uint256 _gas, + uint256 _value, + bytes memory _calldata +) returns (bool success) { + require(hasMinGas(_gas, 1_000), "Must have minGas"); + + // dispatch message to recipient + // by assembly calling "handle" function + // we call via assembly to avoid memcopying a very large returndata + // returned by a malicious contract + assembly { + success := call( + _gas, // gas + _target, // recipient + _value, // ether value + add(_calldata, 0x20), // inloc + mload(_calldata), // inlen + 0, // outloc + 0 // outlen + ) + + // Ignore all return values + } + return (success); +} \ No newline at end of file From fcd26fba000223333a5ebad03bc48de8209249a9 Mon Sep 17 00:00:00 2001 From: gallo Date: Mon, 7 Oct 2024 10:11:41 +0200 Subject: [PATCH 3/9] feat: docs --- src/Governance.sol | 10 ++++++---- src/utils/SafeCallMinGas.sol | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index d60caf0..ef5f850 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -21,6 +21,8 @@ import {safeCallWithMinGas} from "./utils/SafeCallMinGas.sol"; contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance { using SafeERC20 for IERC20; + uint256 constant MIN_GAS_TO_HOOK = 350_000; /// Replace this to ensure hooks have sufficient gas + /// @inheritdoc IGovernance ILQTYStaking public immutable stakingV1; /// @inheritdoc IGovernance @@ -326,7 +328,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance // try IInitiative(_initiative).onRegisterInitiative(currentEpoch) {} catch {} // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch))); + safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch))); } /// @inheritdoc IGovernance @@ -374,7 +376,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance // try IInitiative(_initiative).onUnregisterInitiative(currentEpoch) {} catch {} // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch))); + safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch))); } /// @inheritdoc IGovernance @@ -486,7 +488,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance // currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY // ) {} catch {} // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas(initiative, 350_000, 0, abi.encodeCall(IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY))); + safeCallWithMinGas(initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY))); } require( @@ -518,7 +520,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance // try IInitiative(_initiative).onClaimForInitiative(votesSnapshot_.forEpoch, claim) {} catch {} // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas(_initiative, 350_000, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claim))); + safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claim))); return claim; } diff --git a/src/utils/SafeCallMinGas.sol b/src/utils/SafeCallMinGas.sol index 186b90a..04025c3 100644 --- a/src/utils/SafeCallMinGas.sol +++ b/src/utils/SafeCallMinGas.sol @@ -19,7 +19,9 @@ function safeCallWithMinGas( uint256 _value, bytes memory _calldata ) returns (bool success) { - require(hasMinGas(_gas, 1_000), "Must have minGas"); + /// @audit This is not necessary + /// But this is basically a worst case estimate of mem exp cost + operations before the call + require(hasMinGas(_gas, 1_000), "Must have minGas"); // dispatch message to recipient // by assembly calling "handle" function From 328b8e593b9a66f3b6e65a3c3b66287531a321f0 Mon Sep 17 00:00:00 2001 From: gallo Date: Mon, 7 Oct 2024 15:10:41 +0200 Subject: [PATCH 4/9] fix: require eth success --- src/UserProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UserProxy.sol b/src/UserProxy.sol index 08ae5fb..86f4a8c 100644 --- a/src/UserProxy.sol +++ b/src/UserProxy.sol @@ -75,7 +75,7 @@ contract UserProxy is IUserProxy { ethAmount = address(this).balance; if (ethAmount > 0) { (bool success,) = payable(_lusdEthRecipient).call{value: ethAmount}(""); - success; + require(success, "UserProxy: eth-fail"); } emit Unstake(_amount, _lqtyRecipient, _lusdEthRecipient, lusdAmount, ethAmount); From c6e9dfdc996247a6f97f2b53be813f1794e3cfa4 Mon Sep 17 00:00:00 2001 From: gallo Date: Mon, 7 Oct 2024 17:39:20 +0200 Subject: [PATCH 5/9] feat: curvev2 integration cannot be griefed --- src/CurveV2GaugeRewards.sol | 28 ++++++++++++---- test/CurveV2GaugeRewards.t.sol | 61 +++++++++++++++++++++++++++++----- 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/src/CurveV2GaugeRewards.sol b/src/CurveV2GaugeRewards.sol index 9c6ae51..95282ac 100644 --- a/src/CurveV2GaugeRewards.sol +++ b/src/CurveV2GaugeRewards.sol @@ -18,14 +18,30 @@ contract CurveV2GaugeRewards is BribeInitiative { duration = _duration; } - function depositIntoGauge() external returns (uint256) { - uint256 amount = governance.claimForInitiative(address(this)); + uint256 public remainder; - bold.approve(address(gauge), amount); - gauge.deposit_reward_token(address(bold), amount, duration); - emit DepositIntoGauge(amount); + /// @notice Governance transfers Bold, and we deposit it into the gauge + /// @dev Doing this allows anyone to trigger the claim + function onClaimForInitiative(uint16, uint256 _bold) external override onlyGovernance { + _depositIntoGauge(_bold); + } + + function _depositIntoGauge(uint256 amount) internal { + + // For small donations queue them into the contract + if(amount < duration * 1000) { + remainder += amount; + return; + } + + uint256 total = amount + remainder; + remainder = 0; - return amount; + bold.approve(address(gauge), total); + gauge.deposit_reward_token(address(bold), total, duration); + + emit DepositIntoGauge(total); } + } diff --git a/test/CurveV2GaugeRewards.t.sol b/test/CurveV2GaugeRewards.t.sol index e46a137..fd053d2 100644 --- a/test/CurveV2GaugeRewards.t.sol +++ b/test/CurveV2GaugeRewards.t.sol @@ -42,6 +42,8 @@ contract CurveV2GaugeRewardsTest is Test { ILiquidityGauge private gauge; CurveV2GaugeRewards private curveV2GaugeRewards; + address mockGovernance = address(0x123123); + function setUp() public { vm.createSelectFork(vm.rpcUrl("mainnet"), 20430000); @@ -68,7 +70,7 @@ contract CurveV2GaugeRewardsTest is Test { curveV2GaugeRewards = new CurveV2GaugeRewards( // address(vm.computeCreateAddress(address(this), vm.getNonce(address(this)) + 1)), - address(new MockGovernance()), + address(mockGovernance), address(lusd), address(lqty), address(gauge), @@ -117,14 +119,57 @@ contract CurveV2GaugeRewardsTest is Test { vm.stopPrank(); } - function test_depositIntoGauge() public { - vm.startPrank(lusdHolder); - lusd.transfer(address(curveV2GaugeRewards), 1000e18); - vm.stopPrank(); + function test_claimAndDepositIntoGaugeFuzz(uint128 amt) public { + deal(address(lusd), mockGovernance, amt); + vm.assume(amt > 604800); + + + // Pretend a Proposal has passed + vm.startPrank( + address(mockGovernance) + ); + lusd.transfer(address(curveV2GaugeRewards), amt); + + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), amt); + curveV2GaugeRewards.onClaimForInitiative(0, amt); + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), curveV2GaugeRewards.remainder()); + } + + /// @dev If the amount rounds down below 1 per second it reverts + function test_claimAndDepositIntoGaugeGrief() public { + uint256 amt = 604800 - 1; + deal(address(lusd), mockGovernance, amt); + + + // Pretend a Proposal has passed + vm.startPrank( + address(mockGovernance) + ); + lusd.transfer(address(curveV2GaugeRewards), amt); + + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), amt); + curveV2GaugeRewards.onClaimForInitiative(0, amt); + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), curveV2GaugeRewards.remainder()); + } + + + /// @dev Fuzz test that shows that given a total = amt + dust, the dust is lost permanently + function test_noDustGriefFuzz(uint128 amt, uint128 dust) public { + uint256 total = uint256(amt) + uint256(dust); + deal(address(lusd), mockGovernance, total); + - vm.mockCall( - address(governance), abi.encode(IGovernance.claimForInitiative.selector), abi.encode(uint256(1000e18)) + // Pretend a Proposal has passed + vm.startPrank( + address(mockGovernance) ); - curveV2GaugeRewards.depositIntoGauge(); + // Dust amount + lusd.transfer(address(curveV2GaugeRewards), amt); + // Rest + lusd.transfer(address(curveV2GaugeRewards), dust); + + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), total); + curveV2GaugeRewards.onClaimForInitiative(0, amt); + assertEq(lusd.balanceOf(address(curveV2GaugeRewards)), curveV2GaugeRewards.remainder() + dust); } } From 07fdd43e9c0e9d2442910756fae5f39c42adbabd Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 8 Oct 2024 18:27:31 +0200 Subject: [PATCH 6/9] feat: malicious governance attack tester --- test/GovernanceAttacks.t.sol | 243 +++++++++++++++++++++++++++++ test/mocks/MaliciousInitiative.sol | 77 +++++++++ 2 files changed, 320 insertions(+) create mode 100644 test/GovernanceAttacks.t.sol create mode 100644 test/mocks/MaliciousInitiative.sol diff --git a/test/GovernanceAttacks.t.sol b/test/GovernanceAttacks.t.sol new file mode 100644 index 0000000..191da6d --- /dev/null +++ b/test/GovernanceAttacks.t.sol @@ -0,0 +1,243 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; +import {console} from "forge-std/console.sol"; + +import {IERC20} from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; + +import {IGovernance} from "../src/interfaces/IGovernance.sol"; +import {ILQTY} from "../src/interfaces/ILQTY.sol"; + +import {BribeInitiative} from "../src/BribeInitiative.sol"; +import {Governance} from "../src/Governance.sol"; +import {UserProxy} from "../src/UserProxy.sol"; + +import {PermitParams} from "../src/utils/Types.sol"; + +import {MaliciousInitiative} from "./mocks/MaliciousInitiative.sol"; + + +contract GovernanceTest is Test { + IERC20 private constant lqty = IERC20(address(0x6DEA81C8171D0bA574754EF6F8b412F2Ed88c54D)); + IERC20 private constant lusd = IERC20(address(0x5f98805A4E8be255a32880FDeC7F6728C6568bA0)); + address private constant stakingV1 = address(0x4f9Fbb3f1E99B56e0Fe2892e623Ed36A76Fc605d); + address private constant user = address(0xF977814e90dA44bFA03b6295A0616a897441aceC); + address private constant user2 = address(0x10C9cff3c4Faa8A60cB8506a7A99411E6A199038); + address private constant lusdHolder = address(0xcA7f01403C4989d2b1A9335A2F09dD973709957c); + + uint128 private constant REGISTRATION_FEE = 1e18; + uint128 private constant REGISTRATION_THRESHOLD_FACTOR = 0.01e18; + uint128 private constant UNREGISTRATION_THRESHOLD_FACTOR = 4e18; + uint16 private constant REGISTRATION_WARM_UP_PERIOD = 4; + uint16 private constant UNREGISTRATION_AFTER_EPOCHS = 4; + uint128 private constant VOTING_THRESHOLD_FACTOR = 0.04e18; + uint88 private constant MIN_CLAIM = 500e18; + uint88 private constant MIN_ACCRUAL = 1000e18; + uint32 private constant EPOCH_DURATION = 604800; + uint32 private constant EPOCH_VOTING_CUTOFF = 518400; + + Governance private governance; + address[] private initialInitiatives; + + MaliciousInitiative private maliciousInitiative1; + MaliciousInitiative private maliciousInitiative2; + MaliciousInitiative private maliciousInitiative3; + + function setUp() public { + vm.createSelectFork(vm.rpcUrl("mainnet"), 20430000); + + maliciousInitiative1 = new MaliciousInitiative(); + maliciousInitiative2 = new MaliciousInitiative(); + maliciousInitiative3 = new MaliciousInitiative(); + + initialInitiatives.push(address(maliciousInitiative1)); + + governance = new Governance( + address(lqty), + address(lusd), + stakingV1, + address(lusd), + IGovernance.Configuration({ + registrationFee: REGISTRATION_FEE, + registrationThresholdFactor: REGISTRATION_THRESHOLD_FACTOR, + unregistrationThresholdFactor: UNREGISTRATION_THRESHOLD_FACTOR, + registrationWarmUpPeriod: REGISTRATION_WARM_UP_PERIOD, + unregistrationAfterEpochs: UNREGISTRATION_AFTER_EPOCHS, + votingThresholdFactor: VOTING_THRESHOLD_FACTOR, + minClaim: MIN_CLAIM, + minAccrual: MIN_ACCRUAL, + epochStart: uint32(block.timestamp), + epochDuration: EPOCH_DURATION, + epochVotingCutoff: EPOCH_VOTING_CUTOFF + }), + initialInitiatives + ); + + } + + // forge test --match-test test_deposit_attack -vv + // All calls should never revert due to malicious initiative + function test_all_revert_attacks_hardcoded() public { + uint256 zeroSnapshot = vm.snapshot(); + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); + + // should not revert if the user doesn't have a UserProxy deployed yet + address userProxy = governance.deriveUserProxyAddress(user); + lqty.approve(address(userProxy), 1e18); + + // deploy and deposit 1 LQTY + governance.depositLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 1e18); + (uint88 allocatedLQTY, uint32 averageStakingTimestamp) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + // first deposit should have an averageStakingTimestamp if block.timestamp + assertEq(averageStakingTimestamp, block.timestamp); + vm.warp(block.timestamp + timeIncrease); + vm.stopPrank(); + + + vm.startPrank(lusdHolder); + lusd.transfer(address(governance), 10000e18); + vm.stopPrank(); + + address maliciousWhale = address(0xb4d); + deal(address(lusd), maliciousWhale, 2000e18); + vm.startPrank(maliciousWhale); + lusd.approve(address(governance), type(uint256).max); + + /// === REGISTRATION REVERTS === /// + uint256 registerNapshot = vm.snapshot(); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.THROW); + governance.registerInitiative(address(maliciousInitiative2)); + vm.revertTo(registerNapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.OOG); + governance.registerInitiative(address(maliciousInitiative2)); + vm.revertTo(registerNapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.RETURN_BOMB); + governance.registerInitiative(address(maliciousInitiative2)); + vm.revertTo(registerNapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.REVERT_BOMB); + governance.registerInitiative(address(maliciousInitiative2)); + vm.revertTo(registerNapshot); + + // Reset and continue + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.NONE); + governance.registerInitiative(address(maliciousInitiative2)); + + vm.stopPrank(); + + vm.warp(block.timestamp + governance.EPOCH_DURATION()); + + vm.startPrank(user); + address[] memory initiatives = new address[](1); + initiatives[0] = address(maliciousInitiative2); + int176[] memory deltaVoteLQTY = new int176[](1); + deltaVoteLQTY[0] = 5e17; + int176[] memory deltaVetoLQTY = new int176[](1); + + /// === Allocate LQTY REVERTS === /// + uint256 allocateSnapshot = vm.snapshot(); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.THROW); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + vm.revertTo(allocateSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.OOG); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + vm.revertTo(allocateSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.RETURN_BOMB); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + vm.revertTo(allocateSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.REVERT_BOMB); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + vm.revertTo(allocateSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.NONE); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + + + + vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1); + + /// === Claim for initiative REVERTS === /// + uint256 claimShapsnot = vm.snapshot(); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.THROW); + governance.claimForInitiative(address(maliciousInitiative2)); + vm.revertTo(claimShapsnot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.OOG); + governance.claimForInitiative(address(maliciousInitiative2)); + vm.revertTo(claimShapsnot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.RETURN_BOMB); + governance.claimForInitiative(address(maliciousInitiative2)); + vm.revertTo(claimShapsnot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.REVERT_BOMB); + governance.claimForInitiative(address(maliciousInitiative2)); + vm.revertTo(claimShapsnot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.NONE); + uint256 claimed = governance.claimForInitiative(address(maliciousInitiative2)); + assertEq(claimed, 10001e18); + + + /// === Unregister Reverts === /// + + vm.startPrank(user); + initiatives = new address[](2); + initiatives[0] = address(maliciousInitiative2); + initiatives[1] = address(maliciousInitiative1); + deltaVoteLQTY = new int176[](2); + deltaVoteLQTY[0] = -5e17; + deltaVoteLQTY[1] = 5e17; + deltaVetoLQTY = new int176[](2); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + + (Governance.VoteSnapshot memory v, Governance.InitiativeVoteSnapshot memory initData) = governance.snapshotVotesForInitiative(address(maliciousInitiative2)); + uint256 currentEpoch = governance.epoch(); + assertEq(initData.lastCountedEpoch, currentEpoch - 1, "Epoch Matches"); + + // Inactive for 4 epochs + // Add another proposal + + vm.warp(block.timestamp + governance.EPOCH_DURATION() * 4); + (v, initData) = governance.snapshotVotesForInitiative(address(maliciousInitiative2)); + assertEq(initData.lastCountedEpoch, currentEpoch - 1, "Epoch Matches"); /// @audit This fails if you have 0 votes, see QA + + uint256 unregisterSnapshot = vm.snapshot(); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.THROW); + governance.unregisterInitiative(address(maliciousInitiative2)); + vm.revertTo(unregisterSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.OOG); + governance.unregisterInitiative(address(maliciousInitiative2)); + vm.revertTo(unregisterSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.RETURN_BOMB); + governance.unregisterInitiative(address(maliciousInitiative2)); + vm.revertTo(unregisterSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.REVERT_BOMB); + governance.unregisterInitiative(address(maliciousInitiative2)); + vm.revertTo(unregisterSnapshot); + + maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.NONE); + governance.unregisterInitiative(address(maliciousInitiative2)); + } + + +} diff --git a/test/mocks/MaliciousInitiative.sol b/test/mocks/MaliciousInitiative.sol new file mode 100644 index 0000000..2dbd60c --- /dev/null +++ b/test/mocks/MaliciousInitiative.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; +import {IInitiative} from "src/interfaces/IInitiative.sol"; + +contract MaliciousInitiative is IInitiative { + + enum FunctionType { + NONE, + REGISTER, + UNREGISTER, + ALLOCATE, + CLAIM + } + + enum RevertType { + NONE, + THROW, + OOG, + RETURN_BOMB, + REVERT_BOMB + } + + mapping (FunctionType => RevertType) revertBehaviours; + + /// @dev specify the revert behaviour on each function + function setRevertBehaviour(FunctionType ft, RevertType rt) external { + revertBehaviours[ft] = rt; + } + + // Do stuff on each hook + function onRegisterInitiative(uint16 _atEpoch) external override { + _performRevertBehaviour(revertBehaviours[FunctionType.REGISTER]); + } + + function onUnregisterInitiative(uint16 _atEpoch) external override { + _performRevertBehaviour(revertBehaviours[FunctionType.UNREGISTER]); + } + + + function onAfterAllocateLQTY(uint16 _currentEpoch, address _user, uint88 _voteLQTY, uint88 _vetoLQTY) external override { + _performRevertBehaviour(revertBehaviours[FunctionType.ALLOCATE]); + } + + function onClaimForInitiative(uint16 _claimEpoch, uint256 _bold) external override { + _performRevertBehaviour(revertBehaviours[FunctionType.CLAIM]); + } + + function _performRevertBehaviour(RevertType action) internal { + if(action == RevertType.THROW) { + revert("A normal Revert"); + } + + // 3 gas per iteration, consider changing to storage changes if traces are cluttered + if(action == RevertType.OOG) { + uint256 i; + while(true) { + ++i; + } + } + + if(action == RevertType.RETURN_BOMB) { + uint256 _bytes = 2_000_000; + assembly { + return(0, _bytes) + } + } + + if(action == RevertType.REVERT_BOMB) { + uint256 _bytes = 2_000_000; + assembly { + revert(0, _bytes) + } + } + + return; // NONE + } +} \ No newline at end of file From 08d897c9fe06ea7cccb390cea02ee11fd654b431 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 8 Oct 2024 18:35:14 +0200 Subject: [PATCH 7/9] feat: eoa checks as well --- test/GovernanceAttacks.t.sol | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/test/GovernanceAttacks.t.sol b/test/GovernanceAttacks.t.sol index 191da6d..73a4b0a 100644 --- a/test/GovernanceAttacks.t.sol +++ b/test/GovernanceAttacks.t.sol @@ -43,14 +43,14 @@ contract GovernanceTest is Test { MaliciousInitiative private maliciousInitiative1; MaliciousInitiative private maliciousInitiative2; - MaliciousInitiative private maliciousInitiative3; + MaliciousInitiative private eoaInitiative; function setUp() public { vm.createSelectFork(vm.rpcUrl("mainnet"), 20430000); maliciousInitiative1 = new MaliciousInitiative(); maliciousInitiative2 = new MaliciousInitiative(); - maliciousInitiative3 = new MaliciousInitiative(); + eoaInitiative = MaliciousInitiative(address(0x123123123123)); initialInitiatives.push(address(maliciousInitiative1)); @@ -133,16 +133,22 @@ contract GovernanceTest is Test { maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.NONE); governance.registerInitiative(address(maliciousInitiative2)); + // Register EOA + governance.registerInitiative(address(eoaInitiative)); + + vm.stopPrank(); vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.startPrank(user); - address[] memory initiatives = new address[](1); + address[] memory initiatives = new address[](2); initiatives[0] = address(maliciousInitiative2); - int176[] memory deltaVoteLQTY = new int176[](1); + initiatives[1] = address(eoaInitiative); + int176[] memory deltaVoteLQTY = new int176[](2); deltaVoteLQTY[0] = 5e17; - int176[] memory deltaVetoLQTY = new int176[](1); + deltaVoteLQTY[1] = 5e17; + int176[] memory deltaVetoLQTY = new int176[](2); /// === Allocate LQTY REVERTS === /// uint256 allocateSnapshot = vm.snapshot(); @@ -191,19 +197,22 @@ contract GovernanceTest is Test { maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.NONE); uint256 claimed = governance.claimForInitiative(address(maliciousInitiative2)); - assertEq(claimed, 10001e18); + + governance.claimForInitiative(address(eoaInitiative)); /// === Unregister Reverts === /// vm.startPrank(user); - initiatives = new address[](2); + initiatives = new address[](3); initiatives[0] = address(maliciousInitiative2); - initiatives[1] = address(maliciousInitiative1); - deltaVoteLQTY = new int176[](2); + initiatives[1] = address(eoaInitiative); + initiatives[2] = address(maliciousInitiative1); + deltaVoteLQTY = new int176[](3); deltaVoteLQTY[0] = -5e17; - deltaVoteLQTY[1] = 5e17; - deltaVetoLQTY = new int176[](2); + deltaVoteLQTY[1] = -5e17; + deltaVoteLQTY[2] = 5e17; + deltaVetoLQTY = new int176[](3); governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); (Governance.VoteSnapshot memory v, Governance.InitiativeVoteSnapshot memory initData) = governance.snapshotVotesForInitiative(address(maliciousInitiative2)); @@ -237,7 +246,10 @@ contract GovernanceTest is Test { maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.NONE); governance.unregisterInitiative(address(maliciousInitiative2)); + + governance.unregisterInitiative(address(eoaInitiative)); } + } From bcf897e5fcea601178b14a2973b5c6d59f7e951b Mon Sep 17 00:00:00 2001 From: gallo Date: Thu, 10 Oct 2024 18:33:12 +0200 Subject: [PATCH 8/9] fix: always use `safeTransfer` --- src/UserProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UserProxy.sol b/src/UserProxy.sol index 86f4a8c..38d557c 100644 --- a/src/UserProxy.sol +++ b/src/UserProxy.sol @@ -36,7 +36,7 @@ contract UserProxy is IUserProxy { /// @inheritdoc IUserProxy function stake(uint256 _amount, address _lqtyFrom) public onlyStakingV2 { - lqty.transferFrom(_lqtyFrom, address(this), _amount); + lqty.safeTransferFrom(_lqtyFrom, address(this), _amount); lqty.approve(address(stakingV1), _amount); stakingV1.stake(_amount); emit Stake(_amount, _lqtyFrom); From 5812afed647de54a9de04a9d82c6e57da8283557 Mon Sep 17 00:00:00 2001 From: gallo Date: Thu, 10 Oct 2024 18:43:13 +0200 Subject: [PATCH 9/9] fix: counted ts fix --- src/Governance.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Governance.sol b/src/Governance.sol index ef5f850..7c9a8c9 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -456,7 +456,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance if (prevInitiativeState.counted == 1) { state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( state.countedVoteLQTYAverageTimestamp, - initiativeState.averageStakingTimestampVoteLQTY, + prevInitiativeState.averageStakingTimestampVoteLQTY, state.countedVoteLQTY, state.countedVoteLQTY - prevInitiativeState.voteLQTY );