Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix int to uint casting overflow #3

Merged
merged 5 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 5 additions & 0 deletions src/ForwardBribe.sol
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
16 changes: 11 additions & 5 deletions src/Governance.sol
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions src/UserProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down
7 changes: 2 additions & 5 deletions src/interfaces/IGovernance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/IUserProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
17 changes: 7 additions & 10 deletions src/utils/Math.sol
Original file line number Diff line number Diff line change
@@ -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);
}
4 changes: 2 additions & 2 deletions test/BribeInitiative.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
156 changes: 131 additions & 25 deletions test/Governance.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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");
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
}
Loading
Loading