diff --git a/.gitmodules b/.gitmodules index ee9c92b9..c54835db 100644 --- a/.gitmodules +++ b/.gitmodules @@ -9,4 +9,4 @@ url = https://github.com/Uniswap/v4-core [submodule "lib/solmate"] path = lib/solmate - url = git@github.com:Transmissions11/solmate + url = https://github.com/Transmissions11/solmate diff --git a/src/ForwardBribe.sol b/src/ForwardBribe.sol index 234c7403..b7e4d1b8 100644 --- a/src/ForwardBribe.sol +++ b/src/ForwardBribe.sol @@ -1,9 +1,14 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import {IERC20} from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; +import {SafeERC20} from "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; + import {BribeInitiative} from "./BribeInitiative.sol"; contract ForwardBribe is BribeInitiative { + using SafeERC20 for IERC20; + address public immutable receiver; constructor(address _governance, address _bold, address _bribeToken, address _receiver) diff --git a/src/Governance.sol b/src/Governance.sol index 2f361b3a..e550d711 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import {console} from "forge-std/console.sol"; + import {IERC20} from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; import {SafeERC20} from "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; import {ReentrancyGuard} from "openzeppelin-contracts/contracts/utils/ReentrancyGuard.sol"; @@ -12,10 +14,12 @@ import {ILQTYStaking} from "./interfaces/ILQTYStaking.sol"; import {UserProxy} from "./UserProxy.sol"; import {UserProxyFactory} from "./UserProxyFactory.sol"; -import {add, max} from "./utils/Math.sol"; +import {add, max, abs} from "./utils/Math.sol"; import {Multicall} from "./utils/Multicall.sol"; import {WAD, PermitParams} from "./utils/Types.sol"; +import {SafeCastLib} from "lib/solmate/src/utils/SafeCastLib.sol"; + /// @title Governance: Modular Initiative based Governance contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance { using SafeERC20 for IERC20; @@ -68,6 +72,8 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance /// @inheritdoc IGovernance mapping(address => uint16) public override registeredInitiatives; + error RegistrationFailed(address initiative); + constructor( address _lqty, address _lusd, @@ -375,8 +381,8 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance /// @inheritdoc IGovernance function allocateLQTY( address[] calldata _initiatives, - int176[] calldata _deltaLQTYVotes, - int176[] calldata _deltaLQTYVetos + int88[] calldata _deltaLQTYVotes, + int88[] calldata _deltaLQTYVetos ) external nonReentrant { require( _initiatives.length == _deltaLQTYVotes.length && _initiatives.length == _deltaLQTYVetos.length, @@ -392,8 +398,8 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance for (uint256 i = 0; i < _initiatives.length; i++) { address initiative = _initiatives[i]; - int176 deltaLQTYVotes = _deltaLQTYVotes[i]; - int176 deltaLQTYVetos = _deltaLQTYVetos[i]; + int88 deltaLQTYVotes = _deltaLQTYVotes[i]; + int88 deltaLQTYVetos = _deltaLQTYVetos[i]; // only allow vetoing post the voting cutoff require( diff --git a/src/UserProxy.sol b/src/UserProxy.sol index 08ae5fbb..fee41077 100644 --- a/src/UserProxy.sol +++ b/src/UserProxy.sol @@ -82,8 +82,8 @@ contract UserProxy is IUserProxy { } /// @inheritdoc IUserProxy - function staked() external view returns (uint96) { - return uint96(stakingV1.stakes(address(this))); + function staked() external view returns (uint88) { + return uint88(stakingV1.stakes(address(this))); } receive() external payable {} diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 1a4cb51d..6a80dfd0 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -246,11 +246,8 @@ interface IGovernance { /// @param _initiatives Addresses of the initiatives to allocate to /// @param _deltaLQTYVotes Delta LQTY to allocate to the initiatives as votes /// @param _deltaLQTYVetos Delta LQTY to allocate to the initiatives as vetos - function allocateLQTY( - address[] memory _initiatives, - int176[] memory _deltaLQTYVotes, - int176[] memory _deltaLQTYVetos - ) external; + function allocateLQTY(address[] memory _initiatives, int88[] memory _deltaLQTYVotes, int88[] memory _deltaLQTYVetos) + external; /// @notice Splits accrued funds according to votes received between all initiatives /// @param _initiative Addresse of the initiative diff --git a/src/interfaces/IUserProxy.sol b/src/interfaces/IUserProxy.sol index 126812b7..bd8c041d 100644 --- a/src/interfaces/IUserProxy.sol +++ b/src/interfaces/IUserProxy.sol @@ -47,5 +47,5 @@ interface IUserProxy { returns (uint256 lusdAmount, uint256 ethAmount); /// @notice Returns the current amount LQTY staked by a user in the V1 staking contract /// @return staked Amount of LQTY tokens staked - function staked() external view returns (uint96); + function staked() external view returns (uint88); } diff --git a/src/utils/Math.sol b/src/utils/Math.sol index 2e1d6246..dd608737 100644 --- a/src/utils/Math.sol +++ b/src/utils/Math.sol @@ -1,20 +1,17 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; -function add(uint88 a, int192 b) pure returns (uint88) { +function add(uint88 a, int88 b) pure returns (uint88) { if (b < 0) { - return uint88(a - uint88(uint192(-b))); + return a - abs(b); } - return uint88(a + uint88(uint192(b))); -} - -function sub(uint256 a, int256 b) pure returns (uint128) { - if (b < 0) { - return uint128(a + uint256(-b)); - } - return uint128(a - uint256(b)); + return a + abs(b); } function max(uint256 a, uint256 b) pure returns (uint256) { return a > b ? a : b; } + +function abs(int88 a) pure returns (uint88) { + return a < 0 ? uint88(uint256(-int256(a))) : uint88(a); +} diff --git a/test/BribeInitiative.t.sol b/test/BribeInitiative.t.sol index 6f8df633..b4bb4e42 100644 --- a/test/BribeInitiative.t.sol +++ b/test/BribeInitiative.t.sol @@ -102,9 +102,9 @@ contract BribeInitiativeTest is Test { address[] memory initiatives = new address[](1); initiatives[0] = address(bribeInitiative); - int176[] memory deltaVoteLQTY = new int176[](1); + int88[] memory deltaVoteLQTY = new int88[](1); deltaVoteLQTY[0] = 1e18; - int176[] memory deltaVetoLQTY = new int176[](1); + int88[] memory deltaVetoLQTY = new int88[](1); governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); assertEq(bribeInitiative.totalLQTYAllocatedByEpoch(governance.epoch()), 1e18); assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(user, governance.epoch()), 1e18); diff --git a/test/Governance.t.sol b/test/Governance.t.sol index eb2a5554..7ac8d88e 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -738,7 +738,7 @@ contract GovernanceTest is Test { vm.stopPrank(); } - function test_allocateLQTY() public { + function test_allocateLQTY_single() public { vm.startPrank(user); address userProxy = governance.deployUserProxy(); @@ -753,9 +753,9 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](1); initiatives[0] = baseInitiative1; - int176[] memory deltaLQTYVotes = new int176[](1); - deltaLQTYVotes[0] = 1e18; - int176[] memory deltaLQTYVetos = new int176[](1); + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = 1e18; //this should be 0 + int88[] memory deltaLQTYVetos = new int88[](1); // should revert if the initiative has been registered in the current epoch vm.expectRevert("Governance: initiative-not-active"); @@ -885,10 +885,10 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](2); initiatives[0] = baseInitiative1; initiatives[1] = baseInitiative2; - int176[] memory deltaLQTYVotes = new int176[](2); + int88[] memory deltaLQTYVotes = new int88[](2); deltaLQTYVotes[0] = 1e18; deltaLQTYVotes[1] = 1e18; - int176[] memory deltaLQTYVetos = new int176[](2); + int88[] memory deltaLQTYVetos = new int88[](2); vm.warp(block.timestamp + 365 days); @@ -916,7 +916,7 @@ contract GovernanceTest is Test { } function test_allocateLQTY_fuzz_deltaLQTYVotes(uint88 _deltaLQTYVotes) public { - vm.assume(_deltaLQTYVotes > 0); + vm.assume(_deltaLQTYVotes > 0 && _deltaLQTYVotes < uint88(type(int88).max)); vm.startPrank(user); @@ -928,9 +928,9 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](1); initiatives[0] = baseInitiative1; - int176[] memory deltaLQTYVotes = new int176[](1); - deltaLQTYVotes[0] = int176(uint176(_deltaLQTYVotes)); - int176[] memory deltaLQTYVetos = new int176[](1); + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = int88(uint88(_deltaLQTYVotes)); + int88[] memory deltaLQTYVetos = new int88[](1); vm.warp(block.timestamp + 365 days); @@ -940,7 +940,7 @@ contract GovernanceTest is Test { } function test_allocateLQTY_fuzz_deltaLQTYVetos(uint88 _deltaLQTYVetos) public { - vm.assume(_deltaLQTYVetos > 0); + vm.assume(_deltaLQTYVetos > 0 && _deltaLQTYVetos < uint88(type(int88).max)); vm.startPrank(user); @@ -952,9 +952,9 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](1); initiatives[0] = baseInitiative1; - int176[] memory deltaLQTYVotes = new int176[](1); - int176[] memory deltaLQTYVetos = new int176[](1); - deltaLQTYVetos[0] = int176(uint176(_deltaLQTYVetos)); + int88[] memory deltaLQTYVotes = new int88[](1); + int88[] memory deltaLQTYVetos = new int88[](1); + deltaLQTYVetos[0] = int88(uint88(_deltaLQTYVetos)); vm.warp(block.timestamp + 365 days); @@ -985,10 +985,10 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](2); initiatives[0] = baseInitiative1; initiatives[1] = baseInitiative2; - int176[] memory deltaVoteLQTY = new int176[](2); + int88[] memory deltaVoteLQTY = new int88[](2); deltaVoteLQTY[0] = 500e18; deltaVoteLQTY[1] = 500e18; - int176[] memory deltaVetoLQTY = new int176[](2); + int88[] memory deltaVetoLQTY = new int88[](2); governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); (uint88 allocatedLQTY,) = governance.userStates(user); assertEq(allocatedLQTY, 1000e18); @@ -1036,6 +1036,82 @@ contract GovernanceTest is Test { vm.stopPrank(); } + // this shouldn't happen + function off_claimForInitiativeEOA() public { + address EOAInitiative = address(0xbeef); + + vm.startPrank(user); + + // deploy + address userProxy = governance.deployUserProxy(); + + lqty.approve(address(userProxy), 1000e18); + governance.depositLQTY(1000e18); + + vm.warp(block.timestamp + 365 days); + + vm.stopPrank(); + + vm.startPrank(lusdHolder); + lusd.transfer(address(governance), 10000e18); + vm.stopPrank(); + + vm.startPrank(user); + + address[] memory initiatives = new address[](2); + initiatives[0] = EOAInitiative; // attempt for an EOA + initiatives[1] = baseInitiative2; + int88[] memory deltaVoteLQTY = new int88[](2); + deltaVoteLQTY[0] = 500e18; + deltaVoteLQTY[1] = 500e18; + int88[] memory deltaVetoLQTY = new int88[](2); + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + (uint88 allocatedLQTY,) = governance.userStates(user); + assertEq(allocatedLQTY, 1000e18); + + vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1); + + // should compute the claim and transfer it to the initiative + assertEq(governance.claimForInitiative(EOAInitiative), 5000e18); + governance.claimForInitiative(EOAInitiative); + assertEq(governance.claimForInitiative(EOAInitiative), 0); + assertEq(lusd.balanceOf(EOAInitiative), 5000e18); + + assertEq(governance.claimForInitiative(baseInitiative2), 5000e18); + assertEq(governance.claimForInitiative(baseInitiative2), 0); + + assertEq(lusd.balanceOf(baseInitiative2), 5000e18); + + vm.stopPrank(); + + vm.startPrank(lusdHolder); + lusd.transfer(address(governance), 10000e18); + vm.stopPrank(); + + vm.startPrank(user); + + initiatives[0] = EOAInitiative; + initiatives[1] = baseInitiative2; + deltaVoteLQTY[0] = 495e18; + deltaVoteLQTY[1] = -495e18; + governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY); + + vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1); + + assertEq(governance.claimForInitiative(EOAInitiative), 10000e18); + // should not allow double claiming + assertEq(governance.claimForInitiative(EOAInitiative), 0); + + assertEq(lusd.balanceOf(EOAInitiative), 15000e18); + + assertEq(governance.claimForInitiative(baseInitiative2), 0); + assertEq(governance.claimForInitiative(baseInitiative2), 0); + + assertEq(lusd.balanceOf(baseInitiative2), 5000e18); + + vm.stopPrank(); + } + function test_multicall() public { vm.startPrank(user); @@ -1049,22 +1125,22 @@ contract GovernanceTest is Test { bytes[] memory data = new bytes[](7); address[] memory initiatives = new address[](1); initiatives[0] = baseInitiative1; - int176[] memory deltaVoteLQTY = new int176[](1); - deltaVoteLQTY[0] = int176(uint176(lqtyAmount)); - int176[] memory deltaVetoLQTY = new int176[](1); + int88[] memory deltaVoteLQTY = new int88[](1); + deltaVoteLQTY[0] = int88(uint88(lqtyAmount)); + int88[] memory deltaVetoLQTY = new int88[](1); - int176[] memory deltaVoteLQTY_ = new int176[](1); - deltaVoteLQTY_[0] = -int176(uint176(lqtyAmount)); + int88[] memory deltaVoteLQTY_ = new int88[](1); + deltaVoteLQTY_[0] = -int88(uint88(lqtyAmount)); data[0] = abi.encodeWithSignature("deployUserProxy()"); data[1] = abi.encodeWithSignature("depositLQTY(uint88)", lqtyAmount); data[2] = abi.encodeWithSignature( - "allocateLQTY(address[],int176[],int176[])", initiatives, deltaVoteLQTY, deltaVetoLQTY + "allocateLQTY(address[],int88[],int88[])", initiatives, deltaVoteLQTY, deltaVetoLQTY ); data[3] = abi.encodeWithSignature("userStates(address)", user); data[4] = abi.encodeWithSignature("snapshotVotesForInitiative(address)", baseInitiative1); data[5] = abi.encodeWithSignature( - "allocateLQTY(address[],int176[],int176[])", initiatives, deltaVoteLQTY_, deltaVetoLQTY + "allocateLQTY(address[],int88[],int88[])", initiatives, deltaVoteLQTY_, deltaVetoLQTY ); data[6] = abi.encodeWithSignature("withdrawLQTY(uint88)", lqtyAmount); bytes[] memory response = governance.multicall(data); @@ -1116,8 +1192,8 @@ contract GovernanceTest is Test { address[] memory initiatives = new address[](1); initiatives[0] = address(mockInitiative); - int176[] memory deltaLQTYVotes = new int176[](1); - int176[] memory deltaLQTYVetos = new int176[](1); + int88[] memory deltaLQTYVotes = new int88[](1); + int88[] memory deltaLQTYVetos = new int88[](1); governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); // check that votingThreshold is is high enough such that MIN_CLAIM is met @@ -1171,4 +1247,34 @@ contract GovernanceTest is Test { governance.unregisterInitiative(address(mockInitiative)); } + + // CS exploit PoC + function test_allocateLQTY_overflow() public { + vm.startPrank(user); + + address[] memory initiatives = new address[](2); + initiatives[0] = baseInitiative1; + initiatives[1] = baseInitiative2; + + int88[] memory deltaLQTYVotes = new int88[](2); + deltaLQTYVotes[0] = 0; + deltaLQTYVotes[1] = type(int88).max; + int88[] memory deltaLQTYVetos = new int88[](2); + deltaLQTYVetos[0] = 0; + deltaLQTYVetos[1] = 0; + + vm.warp(block.timestamp + 365 days); + vm.expectRevert("Governance: insufficient-or-unallocated-lqty"); + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); + + deltaLQTYVotes[0] = 0; + deltaLQTYVotes[1] = 0; + deltaLQTYVetos[0] = 0; + deltaLQTYVetos[1] = type(int88).max; + + vm.expectRevert("Governance: insufficient-or-unallocated-lqty"); + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); + + vm.stopPrank(); + } } diff --git a/test/mocks/MockInitiative.sol b/test/mocks/MockInitiative.sol index 8861f420..e0f52603 100644 --- a/test/mocks/MockInitiative.sol +++ b/test/mocks/MockInitiative.sol @@ -24,8 +24,8 @@ contract MockInitiative is IInitiative { /// @inheritdoc IInitiative function onAfterAllocateLQTY(uint16, address, uint88, uint88) external virtual { address[] memory initiatives = new address[](0); - int176[] memory deltaLQTYVotes = new int176[](0); - int176[] memory deltaLQTYVetos = new int176[](0); + int88[] memory deltaLQTYVotes = new int88[](0); + int88[] memory deltaLQTYVetos = new int88[](0); governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); }