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

Audit Fixes - System Changes - Code Refactoring - Mega Merge #17

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
28 changes: 22 additions & 6 deletions src/CurveV2GaugeRewards.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

}
25 changes: 18 additions & 7 deletions src/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@ 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 {
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
Expand Down Expand Up @@ -323,7 +326,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, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch)));
}

/// @inheritdoc IGovernance
Expand Down Expand Up @@ -369,7 +374,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, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch)));
}

/// @inheritdoc IGovernance
Expand Down Expand Up @@ -449,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
);
Expand Down Expand Up @@ -477,9 +484,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, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY)));
}

require(
Expand Down Expand Up @@ -509,7 +518,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, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claim)));

return claim;
}
Expand Down
2 changes: 2 additions & 0 deletions src/UniV4Donations.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/UserProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
44 changes: 44 additions & 0 deletions src/utils/SafeCallMinGas.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// 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) {
/// @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
// 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);
}
61 changes: 53 additions & 8 deletions test/CurveV2GaugeRewards.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);

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