From fc88d1c37394b96d259fe9f7008f0f5536e10a2a Mon Sep 17 00:00:00 2001 From: Pablo Veyrat <50438397+sogipec@users.noreply.github.com> Date: Fri, 10 Nov 2023 12:01:53 +0100 Subject: [PATCH] refactor: comments in the angle-governance system (#2) --- README.md | 9 ++-- contracts/AngleGovernor.sol | 9 ++-- contracts/VeANGLEVotingDelegation.sol | 49 +++++++++---------- .../external/GovernorCountingFractional.sol | 3 ++ contracts/external/GovernorShortCircuit.sol | 17 +++---- contracts/external/GovernorToken.sol | 2 +- contracts/interfaces/IveANGLE.sol | 2 + .../interfaces/IveANGLEVotingDelegation.sol | 5 +- 8 files changed, 50 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 4b3e350..0091263 100644 --- a/README.md +++ b/README.md @@ -19,9 +19,9 @@ It also comes with some utils and scripts to facilitate the creation and executi ## System Architecture 🏘️ -Angle onchain governance works a![Alt text](../angle-governance-old/logo.svg) ../angle-governance-old/README.md ![Alt text](../angle-governance-old/DAO.png)s follows: +Angle onchain governance works as follows: -- veANGLE holders vote on Ethereum on an OpenZeppelin [`Governor`](contracts/AngleGovernor.sol) implementation called `AngleGovernor` with a predetermined quorum, voting delay and proposal threshold. +- veANGLE holders vote on Ethereum on an OpenZeppelin [`Governor`](contracts/AngleGovernor.sol) implementation called `AngleGovernor` with a predetermined quorum, voting delay, proposal and shortcircuit thresholds. - On every chain where the protocol is deployed, there is a `Timelock` contract which is admin of all the protocol contracts (Borrowing module, Transmuter, direct deposit modules, ...) of its chain. - While only onchain votes can lead to payloads being included in the `Timelock` contract of a chain before execution, [Angle 4/6 Governance multisig](https://docs.angle.money/protocol-governance/angle-dao) (deployed on all chains as well) has a veto power on the payloads in Timelock contracts, and can cancel rogue governance votes. - For successful votes on non-Ethereum proposals, payloads to execute are bridged to the chain of interest using LayerZero message passing technology before being sent to the `Timelock` contract of their chain. @@ -53,7 +53,7 @@ It's worth noting that, setup like this, the Angle Governance system can be abst ## Audits -- The `AngleGovernor` implementation relies on several OpenZeppelin extensions as well as on the [audited](http://blog.openzeppelin.com/scopelift-flexible-voting-audit) [`GovernorCountingFractional` extension](https://github.com/ScopeLift/flexible-voting/blob/4399694c1a70d9e236c4c072802bfbe8e4951bf0/src/GovernorCountingFractional.sol) by ScopeLift. +- The `AngleGovernor` implementation relies on several OpenZeppelin extensions, on the [audited](http://blog.openzeppelin.com/scopelift-flexible-voting-audit) [`GovernorCountingFractional` extension](https://github.com/ScopeLift/flexible-voting/blob/4399694c1a70d9e236c4c072802bfbe8e4951bf0/src/GovernorCountingFractional.sol) by ScopeLift. It is a fork of the [audited](https://github.com/trailofbits/publications/blob/master/reviews/2023-05-fraxgov-securityreview.pdf) [governance system by FRAX](https://github.com/FraxFinance/frax-governance). - The [`ProposalReceiver`](contracts/ProposalReceiver.sol) and [`ProposalSender`](contracts/ProposalSender.sol) contracts are forks from LayerZero Labs implementation. Find their audits [here](https://github.com/LayerZero-Labs/omnichain-governance-executor/tree/main/audits). ### Bug Bounty @@ -144,7 +144,7 @@ You can run tests as follows: ```bash forge test -vvvv --watch -forge test -vvvv --match-path contracts/forge-tests/KeeperMulticall.t.sol +forge test -vvvv --match-path test/unit/Constants.t.sol forge test -vvvv --match-test "testAbc*" forge test -vvvv --fork-url https://eth-mainnet.alchemyapi.io/v2/Lc7oIGYeL_QvInzI0Wiu_pOZZDEKBrdf ``` @@ -206,7 +206,6 @@ For any question or feedback you can send an email to [contact@angle.money](mail This repository is released under the [MIT License](LICENSE). - ## Media Don't hesitate to reach out on [Twitter](https://twitter.com/AngleProtocol) 🐦 diff --git a/contracts/AngleGovernor.sol b/contracts/AngleGovernor.sol index eafe7b6..0c96ce6 100644 --- a/contracts/AngleGovernor.sol +++ b/contracts/AngleGovernor.sol @@ -23,8 +23,9 @@ import "./utils/Errors.sol"; /// @dev Core of Angle governance system, extending various OpenZeppelin modules /// @dev This contract overrides some OpenZeppelin function, like those in `GovernorSettings` to introduce /// the `onlyExecutor` modifier which ensures that only the Timelock contract can update the system's parameters -/// @dev The time parameters (`votingDelay`, `votingPeriod`, ...) are expressed here in block number units which -/// means that this implementation is only suited for an Ethereum deployment +/// @dev The time parameters (`votingDelay`, `votingPeriod`, ...) are expressed here in timestamp units, but the +/// also has a `votingDelayBlocks` parameters which must be set in accordance to the `votingDelay` +/// @dev The `state` and `propose` functions here were forked from FRAX governance implementation /// @custom:security-contact contact@angle.money contract AngleGovernor is GovernorSettings, @@ -123,7 +124,7 @@ contract AngleGovernor is /// @inheritdoc Governor // solhint-disable-next-line - /// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/ + /// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/FraxGovernorAlpha.sol function state(uint256 proposalId) public view override returns (ProposalState) { // We read the struct fields into the stack at once so Solidity emits a single SLOAD ProposalCore storage proposal = _proposals[proposalId]; @@ -172,7 +173,7 @@ contract AngleGovernor is /// @inheritdoc Governor // solhint-disable-next-line - /// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/ + /// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/FraxGovernorAlpha.sol function _propose( address[] memory targets, uint256[] memory values, diff --git a/contracts/VeANGLEVotingDelegation.sol b/contracts/VeANGLEVotingDelegation.sol index 5787587..5aa1dc1 100644 --- a/contracts/VeANGLEVotingDelegation.sol +++ b/contracts/VeANGLEVotingDelegation.sol @@ -10,10 +10,11 @@ import { IveANGLE } from "./interfaces/IveANGLE.sol"; import { IveANGLEVotingDelegation } from "./interfaces/IveANGLEVotingDelegation.sol"; /// @title VeANGLEVotingDelegation +/// @notice Contract that keeps track of voting weights and delegations, leveraging veANGLE /// @author Jon Walch (Frax Finance) https://github.com/jonwalch // solhint-disable-next-line -/// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/veANGLEVotingDelegation.sol -/// @notice Contract that keeps track of voting weights and delegations, leveraging veANGLE +/// @dev Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/veANGLEVotingDelegation.sol +/// @dev The Fxs in the variable names and comments have been replaced by ANGLE contract VeANGLEVotingDelegation is EIP712, IERC5805 { using SafeCast for uint256; @@ -54,8 +55,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { VE_ANGLE = IveANGLE(veANGLE); } - /// @notice The ```getCheckpoint``` function is called to retrieve a specific delegation checkpoint - /// @dev Get the checkpoint for a delegate at a given index. + /// @notice Retrieves a specific delegation checkpoint for a delegate at a given index /// @param delegateAddress Address of delegate /// @param index Integer index of the checkpoint /// @return delegateCheckpoint DelegateCheckpoint of ```delegate``` at ```index``` @@ -66,8 +66,8 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { return $delegateCheckpoints[delegateAddress][index]; } - /// @notice The ```_calculateDelegatedWeight``` function calculates weight delegated to ```account``` - /// accounting for any weight that expired since the nearest checkpoint + /// @notice Calculates weight delegated to ```account``` accounting for any weight that expired + /// since the nearest checkpoint /// @dev May include own weight if account previously delegated to someone else and then back to themselves /// @param voter Address of voter /// @param timestamp A block.timestamp, typically corresponding to a proposal snapshot @@ -108,8 +108,8 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { delegatedWeight = expirationAdjustedAngle + (VOTE_WEIGHT_MULTIPLIER * biasAtTimestamp); } - /// @notice The ```_calculateVotingWeight``` function calculates ```account```'s voting weight. - /// Is 0 if they ever delegated and the delegation is in effect. + /// @notice Calculates ```account```'s voting weight. + /// @dev Is 0 if they ever delegated and the delegation is in effect. /// @param voter Address of voter /// @param timestamp A block.timestamp, typically corresponding to a proposal snapshot /// @return votingWeight Voting weight corresponding to ```account```'s veANGLE balance @@ -209,7 +209,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { _calculateDelegatedWeight({ voter: voter, timestamp: timestamp }); } - /// @notice The ```getVotes``` function calculates a voter's weight at ```timestamp``` + /// @notice Calculates a voter's weight at ```timestamp``` /// @param voter Address of voter /// @param timepoint A block.timestamp, typically corresponding to a proposal snapshot /// @return votingWeight Voting weight of ```voterAddress``` at ```timepoint``` @@ -217,14 +217,14 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { return _getVotingWeight({ voter: voter, timestamp: timepoint }); } - /// @notice The ```getVotes``` function calculates a voter's weight at the current block.timestamp + /// @notice Calculates a voter's weight at the current block.timestamp /// @param voter Address of voter /// @return votingWeight Voting weight of ```voterAddress``` at ```block.timestamp``` function getVotes(address voter) external view returns (uint256 votingWeight) { votingWeight = _getVotingWeight({ voter: voter, timestamp: block.timestamp }); } - /// @notice The ```getPastVotes``` function calculates a voter's weight at ```timepoint``` + /// @notice Calculates a voter's weight at ```timepoint``` /// @param voter Address of voter /// @param timepoint A block.timestamp, typically corresponding to a proposal snapshot, must be in past /// @return pastVotingWeight Voting weight of ```account``` at ```timepoint``` in past @@ -249,20 +249,20 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { pastTotalSupply = VE_ANGLE.totalSupplyAt(blockNumber); } - /// @notice The ```delegates``` function returns who the address of the delegate, that delegatorAddress has chosen + /// @notice Returns who the address of the delegate, that delegatorAddress has chosen /// @param delegator Address of delegator /// @return delegateAddress Address of the delegate function delegates(address delegator) external view returns (address delegateAddress) { delegateAddress = $delegations[delegator].delegate; } - /// @notice The ```delegate``` function delegates votes from signer to ```delegatee``` at the next epoch + /// @notice Delegates votes from signer to ```delegatee``` at the next epoch /// @param delegatee Address to delegate to function delegate(address delegatee) external { _delegate({ delegator: msg.sender, delegatee: delegatee }); } - /// @notice The ```delegate``` function delegates votes from signer to ```delegatee``` + /// @notice Delegates votes from signer to ```delegatee``` /// @param delegatee Address to delegate to /// @param nonce Nonce of signed message /// @param expiry Expiry time of message @@ -292,7 +292,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { _delegate(signer, delegatee); } - /// @notice The ```_delegate``` function delegates votes from signer to ```delegatee``` at the next epoch + /// @notice Delegates votes from signer to ```delegatee``` at the next epoch /// @param delegator Caller delegating their weight /// @param delegatee Address to delegate to /// @dev An account can only delegate to one account at a time. The previous delegation will be overwritten. @@ -373,7 +373,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { normalizedVeANGLELockInfo.bias = SafeCast.toUint256(userBias) + normalizedVeANGLELockInfo.slope * lastUpdate; } - /// @notice Do a binary search for the closest checkpoint before ```timestamp``` + /// @notice Does a binary search for the closest checkpoint before ```timestamp``` /// @param _$checkpoints Storage pointer to the account's DelegateCheckpoints /// @param timestamp block.timestamp to get voting power at, frequently proposal snapshot /// @return closestCheckpoint The closest DelegateCheckpoint before timestamp @@ -411,8 +411,8 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { closestCheckpoint = high == 0 ? closestCheckpoint : _$checkpoints[high - 1]; } - /// @notice Removes voting power from the previous delegate, handling expirations - /// @notice and writing a new DelegateCheckpoint + /// @notice Removes voting power from the previous delegate, handling expirations and writing + /// a new DelegateCheckpoint /// @param previousDelegation The delegator's previous delegation /// @param checkpointTimestamp block.timestamp of the next DelegateCheckpoint's epoch function _moveVotingPowerFromPreviousDelegate( @@ -488,8 +488,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { } } - /// @notice Add voting power to the new delegate, handling expirations - /// @notice and writing a new DelegateCheckpoint + /// @notice Adds voting power to the new delegate, handling expirations and writing a new DelegateCheckpoint /// @param newDelegate The new delegate that is being delegated to /// @param delegatorVeANGLELockInfo Information about the delegator's veANGLE lock /// @param checkpointTimestamp block.timestamp of the next DelegateCheckpoint's epoch @@ -553,7 +552,7 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { }); } - /// @notice Calculate the values to be written for the new DelegateCheckpoint + /// @notice Calculates the values to be written for the new DelegateCheckpoint /// @param previousCheckpoint The previous checkpoint for account /// @param account The account to calculate the expirations for /// @param isDeltaPositive Whether adding or subtracting from the previous checkpoint @@ -627,8 +626,8 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { } } - /// @notice Push a new checkpoint to the array - /// or updates the most recent one if it already exists for the current epoch + /// @notice Pushes a new checkpoint to the array or updates the most recent one if it already + /// exists for the current epoch /// @param $userDelegationCheckpoints Pointer to the user's delegation checkpoints /// @param accountCheckpointsLength The length of the user's delegation checkpoints /// @param newCheckpoint The new checkpoint returned from _calculateCheckpoint() @@ -648,8 +647,8 @@ contract VeANGLEVotingDelegation is EIP712, IERC5805 { } } - /// @notice Generate a summation of all bias, slope, and angle - /// for all delegations that expire during the specified time window for ```account``` + /// @notice Generates a summation of all bias, slope, and angle for all delegations that expire during the + /// specified time window for ```account``` /// @param account Delegate account to generate summations for /// @param start Timestamp to start the summations from. The start is not included /// @param end Timestamp to end the summations. The end is included diff --git a/contracts/external/GovernorCountingFractional.sol b/contracts/external/GovernorCountingFractional.sol index e57c276..af68dec 100644 --- a/contracts/external/GovernorCountingFractional.sol +++ b/contracts/external/GovernorCountingFractional.sol @@ -18,7 +18,10 @@ import "../utils/Errors.sol"; * voting with tokens that are held by a DeFi pool, voting from L2 with tokens * held by a bridge, or voting privately from a shielded pool using zero * knowledge proofs. + * @author ScopeLift */ +//solhint-disable-next-line +// Fork from: https://github.com/ScopeLift/flexible-voting/blob/4399694c1a70d9e236c4c072802bfbe8e4951bf0/src/GovernorCountingFractional.sol abstract contract GovernorCountingFractional is Governor { struct ProposalVote { uint128 againstVotes; diff --git a/contracts/external/GovernorShortCircuit.sol b/contracts/external/GovernorShortCircuit.sol index 571092e..dd0c5a1 100644 --- a/contracts/external/GovernorShortCircuit.sol +++ b/contracts/external/GovernorShortCircuit.sol @@ -11,12 +11,11 @@ import { GovernorCountingFractional, SafeCast } from "./GovernorCountingFraction import "../utils/Errors.sol"; /// @title GovernorShortCircuit +/// @notice Extends governor to pass propositions if the quorum is reached before the end of the voting period /// @author Angle Labs, Inc /// @author Jon Walch (Frax Finance) https://github.com/jonwalch +//solhint-disable-next-line /// @notice https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/veANGLEVotingDelegation.sol -/// @notice Contract extending governor to pass propositions -/// if the quorum is reached before the end of the voting period -/// @dev We modified it to only work with block.number and not block.timestamp abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractional, GovernorVotesQuorumFraction { using SafeCast for *; using Checkpoints for Checkpoints.Trace224; @@ -61,14 +60,14 @@ abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractio VIEW FUNCTIONS //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ - /// @notice The ```shortCircuitNumerator``` function returns the latest short circuit numerator + /// @notice Returns the latest short circuit numerator /// @dev Mirrors ```GovernorVotesQuorumFraction::quorumNumerator()``` /// @return latestShortCircuitNumerator The short circuit numerator function shortCircuitNumerator() public view returns (uint256 latestShortCircuitNumerator) { latestShortCircuitNumerator = _$shortCircuitNumeratorHistory.latest(); } - /// @notice The ```shortCircuitNumerator``` function returns the short circuit numerator at ```timepoint``` + /// @notice Returns the short circuit numerator at ```timepoint``` /// @dev Mirrors ```GovernorVotesQuorumFraction::quorumNumerator(uint256 timepoint)``` /// @param timepoint A block.number /// @return shortCircuitNumeratorAtTimepoint Short circuit numerator @@ -89,7 +88,7 @@ abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractio ); } - /// @notice The ```shortCircuitThreshold``` function returns the latest short circuit numerator + /// @notice Returns the latest short circuit numerator /// @dev Only supports historical quorum values for proposals that actually exist at ```timepoint``` /// @param timepoint A block.number corresponding to a proposal snapshot /// @return shortCircuitThresholdAtTimepoint Total voting weight needed for short circuit to succeed @@ -106,7 +105,7 @@ abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractio INTERNALS //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ - /// @notice The ```_shortCircuitFor``` function is called by state() to check for early proposal success + /// @notice Called by state() to check for early proposal success /// @param proposalId Proposal ID /// @return isShortCircuitFor Represents if short circuit threshold for votes were reached or not function _shortCircuitFor(uint256 proposalId) internal view returns (bool isShortCircuitFor) { @@ -116,7 +115,7 @@ abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractio isShortCircuitFor = forVoteWeight > shortCircuitThreshold(proposalVoteStart); } - /// @notice The ```_shortCircuitAgainst``` function is called by state() to check for early proposal failure + /// @notice Called by state() to check for early proposal failure /// @param proposalId Proposal ID /// @return isShortCircuitAgainst Represents if short circuit threshold against votes were reached or not function _shortCircuitAgainst(uint256 proposalId) internal view returns (bool isShortCircuitAgainst) { @@ -126,7 +125,7 @@ abstract contract GovernorShortCircuit is GovernorVotes, GovernorCountingFractio isShortCircuitAgainst = againstVoteWeight > shortCircuitThreshold(proposalVoteStart); } - /// @notice The ```_updateShortCircuitNumerator``` function is called by governance to change the short circuit numerator + /// @notice Called by governance to change the short circuit numerator /// @dev Mirrors ```GovernorVotesQuorumFraction::_updateQuorumNumerator(uint256 newQuorumNumerator)``` /// @param newShortCircuitNumerator New short circuit numerator value function _updateShortCircuitNumerator(uint256 newShortCircuitNumerator) internal { diff --git a/contracts/external/GovernorToken.sol b/contracts/external/GovernorToken.sol index 5a1b6d3..62c19ae 100644 --- a/contracts/external/GovernorToken.sol +++ b/contracts/external/GovernorToken.sol @@ -22,7 +22,7 @@ abstract contract GovernorToken is GovernorVotes { return _token_; } - /// @param veANGLEVotingDelegation new IERC5805 VeANGLEVotingDelegation contract address + /// @param veANGLEVotingDelegation New IERC5805 VeANGLEVotingDelegation contract address function _setVeANGLEVotingDelegation(address veANGLEVotingDelegation) internal { address oldVeANGLEVotingDelegation = address(token()); _token_ = IERC5805(veANGLEVotingDelegation); diff --git a/contracts/interfaces/IveANGLE.sol b/contracts/interfaces/IveANGLE.sol index 665908a..70cd289 100644 --- a/contracts/interfaces/IveANGLE.sol +++ b/contracts/interfaces/IveANGLE.sol @@ -2,6 +2,8 @@ pragma solidity ^0.8.20; +/// @title IveANGLE +/// @notice Interface for the veANGLE contract //solhint-disable interface IveANGLE { struct LockedBalance { diff --git a/contracts/interfaces/IveANGLEVotingDelegation.sol b/contracts/interfaces/IveANGLEVotingDelegation.sol index aceea89..cdb6c25 100644 --- a/contracts/interfaces/IveANGLEVotingDelegation.sol +++ b/contracts/interfaces/IveANGLEVotingDelegation.sol @@ -3,10 +3,11 @@ pragma solidity ^0.8.20; //solhint-disable -/// @title VeANGLEVotingDelegation +/// @title IveANGLEVotingDelegation +/// @notice Interface for the contract that keeps track of voting weights and delegations, leveraging veANGLE /// @author Frax Finance https://github.com/FraxFinance +//solhint-disable-next-line /// @notice Fork from Frax Finance: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/interfaces/IVeFxsVotingDelegation.sol -/// @notice Contract that keeps track of voting weights and delegations, leveraging veANGLE interface IveANGLEVotingDelegation { /// Represents the values of a single delegation at the time `delegate()` is called, /// to be subtracted when removing delegation