Skip to content

Commit

Permalink
Merge pull request #90 from liquity/address-chainsecurity-questions
Browse files Browse the repository at this point in the history 
Address ChainSecurity questions
  • Loading branch information
@danielattilasimon
danielattilasimon authored Dec 3, 2024
2 parents 49776e9 + a1cb319 commit 48afaad
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 108 deletions.
2 changes: 0 additions & 2 deletions src/BribeInitiative.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,6 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
IGovernance.Allocation calldata _allocation,
IGovernance.InitiativeState calldata _initiativeState
) external virtual onlyGovernance {
if (_currentEpoch == 0) return;

uint16 mostRecentUserEpoch = lqtyAllocationByUserAtEpoch[_user].getHead();
uint16 mostRecentTotalEpoch = totalLQTYAllocationByEpoch.getHead();

Expand Down
58 changes: 14 additions & 44 deletions src/Governance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

MIN_CLAIM = _config.minClaim;
MIN_ACCRUAL = _config.minAccrual;
require(_config.epochStart <= block.timestamp, "Gov: cannot-start-in-future");
EPOCH_START = _config.epochStart;
require(_config.epochDuration > 0, "Gov: epoch-duration-zero");
EPOCH_DURATION = _config.epochDuration;
Expand All @@ -132,48 +133,23 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
}

function _averageAge(uint120 _currentTimestamp, uint120 _averageTimestamp) internal pure returns (uint120) {
if (_averageTimestamp == 0 || _currentTimestamp < _averageTimestamp) return 0;
// Due to rounding error, _averageTimestamp can sometimes be higher than _currentTimestamp
if (_currentTimestamp < _averageTimestamp) return 0;
return _currentTimestamp - _averageTimestamp;
}

function _calculateAverageTimestamp(
uint120 _prevOuterAverageTimestamp,
uint120 _newInnerAverageTimestamp,
uint88 _prevLQTYBalance,
uint88 _newLQTYBalance
) internal view returns (uint120) {
uint256 _prevOuterAverageTimestamp,
uint256 _newInnerAverageTimestamp,
uint256 _prevLQTYBalance,
uint256 _newLQTYBalance
) internal pure returns (uint120) {
if (_newLQTYBalance == 0) return 0;

// NOTE: Truncation
// NOTE: u32 -> u120
// While we upscale the Timestamp, the system will stop working at type(uint32).max
// Because the rest of the type is used for precision
uint120 currentTime = uint120(uint32(block.timestamp)) * uint120(TIMESTAMP_PRECISION);

uint120 prevOuterAverageAge = _averageAge(currentTime, _prevOuterAverageTimestamp);
uint120 newInnerAverageAge = _averageAge(currentTime, _newInnerAverageTimestamp);

// 120 for timestamps = 2^32 * 1e18 | 2^32 * 1e26
// 208 for voting power = 2^120 * 2^88
// NOTE: 208 / X can go past u120!
// Therefore we keep `newOuterAverageAge` as u208
uint208 newOuterAverageAge;
if (_prevLQTYBalance <= _newLQTYBalance) {
uint88 deltaLQTY = _newLQTYBalance - _prevLQTYBalance;
uint208 prevVotes = uint208(_prevLQTYBalance) * uint208(prevOuterAverageAge);
uint208 newVotes = uint208(deltaLQTY) * uint208(newInnerAverageAge);
uint208 votes = prevVotes + newVotes;
newOuterAverageAge = votes / _newLQTYBalance;
} else {
uint88 deltaLQTY = _prevLQTYBalance - _newLQTYBalance;
uint208 prevVotes = uint208(_prevLQTYBalance) * uint208(prevOuterAverageAge);
uint208 newVotes = uint208(deltaLQTY) * uint208(newInnerAverageAge);
uint208 votes = (prevVotes >= newVotes) ? prevVotes - newVotes : 0;
newOuterAverageAge = votes / _newLQTYBalance;
}

if (newOuterAverageAge > currentTime) return 0;
return uint120(currentTime - newOuterAverageAge);
return uint120(
_newInnerAverageTimestamp + _prevOuterAverageTimestamp * _prevLQTYBalance / _newLQTYBalance
- _newInnerAverageTimestamp * _prevLQTYBalance / _newLQTYBalance
);
}

/*//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -271,22 +247,16 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

/// @inheritdoc IGovernance
function epoch() public view returns (uint16) {
if (block.timestamp < EPOCH_START) {
return 0;
}
return uint16(((block.timestamp - EPOCH_START) / EPOCH_DURATION) + 1);
}

/// @inheritdoc IGovernance
function epochStart() public view returns (uint32) {
uint16 currentEpoch = epoch();
if (currentEpoch == 0) return 0;
return uint32(EPOCH_START + (currentEpoch - 1) * EPOCH_DURATION);
return uint32(EPOCH_START + (epoch() - 1) * EPOCH_DURATION);
}

/// @inheritdoc IGovernance
function secondsWithinEpoch() public view returns (uint32) {
if (block.timestamp < EPOCH_START) return 0;
return uint32((block.timestamp - EPOCH_START) % EPOCH_DURATION);
}

Expand Down Expand Up @@ -729,7 +699,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0);

/// === Check FSM === ///
// Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states
// Can vote positively in SKIP, CLAIMABLE and CLAIMED states
// Force to remove votes if disabled
// Can remove votes and vetos in every stage
(InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) =
Expand Down
63 changes: 3 additions & 60 deletions test/Governance.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,32 +24,6 @@ import {MockStakingV1} from "./mocks/MockStakingV1.sol";
import {MockStakingV1Deployer} from "./mocks/MockStakingV1Deployer.sol";
import "./constants.sol";

contract GovernanceInternal is Governance {
constructor(
address _lqty,
address _lusd,
address _stakingV1,
address _bold,
Configuration memory _config,
address[] memory _initiatives
) Governance(_lqty, _lusd, _stakingV1, _bold, _config, msg.sender, _initiatives) {}

function averageAge(uint120 _currentTimestamp, uint120 _averageTimestamp) external pure returns (uint120) {
return _averageAge(_currentTimestamp, _averageTimestamp);
}

function calculateAverageTimestamp(
uint120 _prevOuterAverageTimestamp,
uint120 _newInnerAverageTimestamp,
uint88 _prevLQTYBalance,
uint88 _newLQTYBalance
) external view returns (uint208) {
return _calculateAverageTimestamp(
_prevOuterAverageTimestamp, _newInnerAverageTimestamp, _prevLQTYBalance, _newLQTYBalance
);
}
}

abstract contract GovernanceTest is Test {
ILQTY internal lqty;
ILUSD internal lusd;
Expand All @@ -70,7 +44,6 @@ abstract contract GovernanceTest is Test {
uint32 private constant EPOCH_VOTING_CUTOFF = 518400;

Governance private governance;
GovernanceInternal private governanceInternal;
address[] private initialInitiatives;

address private baseInitiative2;
Expand Down Expand Up @@ -108,36 +81,6 @@ abstract contract GovernanceTest is Test {
initialInitiatives.push(baseInitiative1);
initialInitiatives.push(baseInitiative2);
governance.registerInitialInitiatives(initialInitiatives);

governanceInternal = new GovernanceInternal(
address(lqty), address(lusd), address(stakingV1), address(lusd), config, initialInitiatives
);
}

// should not revert under any input
function test_averageAge(uint120 _currentTimestamp, uint120 _timestamp) public {
uint120 averageAge = governanceInternal.averageAge(_currentTimestamp, _timestamp);
if (_timestamp == 0 || _currentTimestamp < _timestamp) {
assertEq(averageAge, 0);
} else {
assertEq(averageAge, _currentTimestamp - _timestamp);
}
}

// should not revert under any input
function test_calculateAverageTimestamp(
uint32 _prevOuterAverageTimestamp,
uint32 _newInnerAverageTimestamp,
uint88 _prevLQTYBalance,
uint88 _newLQTYBalance
) public {
uint32 highestTimestamp = (_prevOuterAverageTimestamp > _newInnerAverageTimestamp)
? _prevOuterAverageTimestamp
: _newInnerAverageTimestamp;
if (highestTimestamp > block.timestamp) vm.warp(highestTimestamp);
governanceInternal.calculateAverageTimestamp(
_prevOuterAverageTimestamp, _newInnerAverageTimestamp, _prevLQTYBalance, _newLQTYBalance
);
}

// forge test --match-test test_depositLQTY_withdrawLQTY -vv
Expand Down Expand Up @@ -315,7 +258,7 @@ abstract contract GovernanceTest is Test {

// should not revert under any block.timestamp >= EPOCH_START
function test_epoch_fuzz(uint32 _timestamp) public {
vm.warp(_timestamp);
vm.warp(governance.EPOCH_START() + _timestamp);
governance.epoch();
}

Expand All @@ -328,7 +271,7 @@ abstract contract GovernanceTest is Test {

// should not revert under any block.timestamp >= EPOCH_START
function test_epochStart_fuzz(uint32 _timestamp) public {
vm.warp(_timestamp);
vm.warp(governance.EPOCH_START() + _timestamp);
governance.epochStart();
}

Expand All @@ -347,7 +290,7 @@ abstract contract GovernanceTest is Test {

// should not revert under any block.timestamp
function test_secondsWithinEpoch_fuzz(uint32 _timestamp) public {
vm.warp(_timestamp);
vm.warp(governance.EPOCH_START() + _timestamp);
governance.secondsWithinEpoch();
}

Expand Down
3 changes: 1 addition & 2 deletions test/recon/CryticToFoundry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts {
console.log("votedPowerSum", votedPowerSum);
console.log("govPower", govPower);

// XXX letting broken property pass for now, so we have green CI status
assertFalse(optimize_property_sum_of_initatives_matches_total_votes_insolvency());
assertTrue(optimize_property_sum_of_initatives_matches_total_votes_insolvency());
}
}

0 comments on commit 48afaad

Please sign in to comment.