Skip to content

Commit

Permalink
Merge branch 'dg-audit' into 3.11-Safer-compare-branching
Browse files Browse the repository at this point in the history
  • Loading branch information
@cairoeth
cairoeth committed Nov 26, 2024
2 parents aba3478 + db92e5d commit aa946e1
Show file tree
Hide file tree
Showing 11 changed files with 172 additions and 89 deletions.
6 changes: 1 addition & 5 deletions src/AgoraGovernor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,22 +126,18 @@ contract AgoraGovernor is
* @param _manager Manager address.
* @param _timelock The governance timelock.
* @param _proposalTypesConfigurator Proposal types configurator contract.
* @param _proposalTypes Initial proposal types to set.
*/
function initialize(
IVotingToken _votingToken,
SupplyType _supplyType,
address _admin,
address _manager,
TimelockControllerUpgradeable _timelock,
IProposalTypesConfigurator _proposalTypesConfigurator,
IProposalTypesConfigurator.ProposalType[] calldata _proposalTypes
IProposalTypesConfigurator _proposalTypesConfigurator
) public initializer {
PROPOSAL_TYPES_CONFIGURATOR = _proposalTypesConfigurator;
SUPPLY_TYPE = _supplyType;

PROPOSAL_TYPES_CONFIGURATOR.initialize(address(this), _proposalTypes);

__Governor_init("Agora");
__GovernorCountingSimple_init();
__GovernorVotes_init(_votingToken);
Expand Down
52 changes: 37 additions & 15 deletions src/ProposalTypesConfigurator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity ^0.8.19;

import {IProposalTypesConfigurator} from "src/interfaces/IProposalTypesConfigurator.sol";
import {IAgoraGovernor} from "src/interfaces/IAgoraGovernor.sol";
import {Validator} from "src/Validator.sol";
import {Validator} from "src/lib/Validator.sol";

/**
* Contract that stores proposalTypes for the Agora Governor.
Expand All @@ -16,15 +16,18 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {

event ScopeCreated(uint8 indexed proposalTypeId, bytes24 indexed scopeKey, bytes4 selector, string description);
event ScopeDisabled(uint8 indexed proposalTypeId, bytes24 indexed scopeKey);
event ScopeDeleted(uint8 indexed proposalTypeId, bytes24 indexed scopeKey);

/*//////////////////////////////////////////////////////////////
IMMUTABLE STORAGE
//////////////////////////////////////////////////////////////*/

IAgoraGovernor public governor;
IAgoraGovernor public immutable GOVERNOR;

/// @notice Max value of `quorum` and `approvalThreshold` in `ProposalType`
uint16 public constant PERCENT_DIVISOR = 10_000;
// @notice Max length of the `assignedScopes` array
uint8 public constant MAX_SCOPE_LENGTH = 5;

/*//////////////////////////////////////////////////////////////
STORAGE
Expand All @@ -39,24 +42,23 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
//////////////////////////////////////////////////////////////*/

modifier onlyAdminOrTimelock() {
if (msg.sender != governor.admin() && msg.sender != governor.timelock()) revert NotAdminOrTimelock();
if (msg.sender != GOVERNOR.admin() && msg.sender != GOVERNOR.timelock()) revert NotAdminOrTimelock();
_;
}

/*//////////////////////////////////////////////////////////////
FUNCTIONS
CONSTRUCTOR
//////////////////////////////////////////////////////////////*/

/**
* @notice Initialize the contract with the governor and proposal types.
* @param _governor Address of the governor contract.
* @param _proposalTypesInit Array of ProposalType structs to initialize the contract with.
*/
function initialize(address _governor, ProposalType[] calldata _proposalTypesInit) external {
if (address(governor) != address(0)) revert AlreadyInit();
constructor(address _governor, ProposalType[] memory _proposalTypesInit) {
if (_governor == address(0)) revert InvalidGovernor();

governor = IAgoraGovernor(_governor);
GOVERNOR = IAgoraGovernor(_governor);

for (uint8 i = 0; i < _proposalTypesInit.length; i++) {
_setProposalType(
Expand All @@ -70,6 +72,10 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
}
}

/*//////////////////////////////////////////////////////////////
FUNCTIONS
//////////////////////////////////////////////////////////////*/

/**
* @notice Get the parameters for a proposal type.
* @param proposalTypeId Id of the proposal type.
Expand Down Expand Up @@ -119,6 +125,7 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
) external override onlyAdminOrTimelock {
if (!_proposalTypes[proposalTypeId].exists) revert InvalidProposalType();
if (parameters.length != comparators.length) revert InvalidParameterConditions();
if (_assignedScopes[proposalTypeId][key].length == MAX_SCOPE_LENGTH) revert MaxScopeLengthReached();

Scope memory scope = Scope(key, selector, parameters, comparators, types, proposalTypeId, description, true);
_assignedScopes[proposalTypeId][key].push(scope);
Expand Down Expand Up @@ -151,16 +158,16 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
uint8 proposalTypeId,
uint16 quorum,
uint16 approvalThreshold,
string calldata name,
string calldata description,
string memory name,
string memory description,
address module
) internal {
if (quorum > PERCENT_DIVISOR) revert InvalidQuorum();
if (approvalThreshold > PERCENT_DIVISOR) revert InvalidApprovalThreshold();

_proposalTypes[proposalTypeId] = ProposalType(quorum, approvalThreshold, name, description, module, true);

emit ProposalTypeSet(proposalTypeId, quorum, approvalThreshold, name, description);
emit ProposalTypeSet(proposalTypeId, quorum, approvalThreshold, name, description, module);
}

/**
Expand All @@ -175,7 +182,7 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
{
if (!_proposalTypes[proposalTypeId].exists) revert InvalidProposalType();
if (scope.parameters.length != scope.comparators.length) revert InvalidParameterConditions();
bytes24 scopeKey = scope.key;
if (_assignedScopes[proposalTypeId][scope.key].length == MAX_SCOPE_LENGTH) revert MaxScopeLengthReached();

_scopeExists[scope.key] = true;
_assignedScopes[proposalTypeId][scope.key].push(scope);
Expand Down Expand Up @@ -205,6 +212,17 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
emit ScopeDisabled(proposalTypeId, scopeKey);
}

/**
* @notice Deletes a scope inside assignedScopes for a proposal type.
* @param proposalTypeId the proposal type ID that has the assigned scope.
* @param scopeKey the contract and function signature representing the scope key
* @param idx the index of the assigned scope.
*/
function deleteScope(uint8 proposalTypeId, bytes24 scopeKey, uint8 idx) external override onlyAdminOrTimelock {
delete _assignedScopes[proposalTypeId][scopeKey][idx];
emit ScopeDeleted(proposalTypeId, scopeKey);
}

/**
* @notice Validates that a proposed transaction conforms to the scope defined in a given proposal type. Note: This
* version only supports functions that have for each parameter 32-byte abi encodings, please see the ABI
Expand All @@ -219,11 +237,11 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
function validateProposedTx(bytes calldata proposedTx, uint8 proposalTypeId, bytes24 key) public view {
Scope[] memory scopes = _assignedScopes[proposalTypeId][key];

if (_scopeExists[key]) {
for (uint8 i = 0; i < scopes.length; i++) {
Scope memory validScope = scopes[i];
if (validScope.selector != bytes4(proposedTx[:4])) revert Invalid4ByteSelector();
for (uint8 i = 0; i < scopes.length; i++) {
Scope memory validScope = scopes[i];
if (validScope.selector != bytes4(proposedTx[:4])) revert Invalid4ByteSelector();

if (validScope.exists) {
uint256 startIdx = 4;
uint256 endIdx = startIdx;
for (uint8 j = 0; j < validScope.parameters.length; j++) {
Expand Down Expand Up @@ -253,7 +271,11 @@ contract ProposalTypesConfigurator is IProposalTypesConfigurator {
external
view
{
if (calldatas.length == 0) revert InvalidCalldatasLength();

for (uint8 i = 0; i < calldatas.length; i++) {
if (calldatas[i].length < 4) revert InvalidCalldata();

bytes24 scopeKey = _pack(targets[i], bytes4(calldatas[i]));
if (_assignedScopes[proposalTypeId][scopeKey].length != 0) {
validateProposedTx(calldatas[i], proposalTypeId, scopeKey);
Expand Down
14 changes: 10 additions & 4 deletions src/interfaces/IProposalTypesConfigurator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,26 @@ interface IProposalTypesConfigurator {
error InvalidScope();
error NotAdminOrTimelock();
error NotAdmin();
error AlreadyInit();
error InvalidGovernor();
error Invalid4ByteSelector();
error InvalidParamNotEqual();
error InvalidParamRange();
error InvalidProposedTxForType();
error MaxScopeLengthReached();
error InvalidCalldatasLength();
error InvalidCalldata();

/*//////////////////////////////////////////////////////////////
EVENTS
//////////////////////////////////////////////////////////////*/

event ProposalTypeSet(
uint8 indexed proposalTypeId, uint16 quorum, uint16 approvalThreshold, string name, string description
uint8 indexed proposalTypeId,
uint16 quorum,
uint16 approvalThreshold,
string name,
string description,
address indexed module
);

/*//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -74,8 +81,6 @@ interface IProposalTypesConfigurator {
FUNCTIONS
//////////////////////////////////////////////////////////////*/

function initialize(address _governor, ProposalType[] calldata _proposalTypes) external;

function proposalTypes(uint8 proposalTypeId) external view returns (ProposalType memory);
function assignedScopes(uint8 proposalTypeId, bytes24 scopeKey) external view returns (Scope[] memory);
function scopeExists(bytes24 key) external view returns (bool);
Expand All @@ -102,6 +107,7 @@ interface IProposalTypesConfigurator {
function getSelector(uint8 proposalTypeId, bytes24 key) external returns (bytes4);
function addScopeForProposalType(uint8 proposalTypeId, Scope calldata scope) external;
function disableScope(uint8 proposalTypeId, bytes24 scopeKey, uint8 idx) external;
function deleteScope(uint8 proposalTypeId, bytes24 scopeKey, uint8 idx) external;
function validateProposedTx(bytes calldata proposedTx, uint8 proposalTypeId, bytes24 key) external;
function validateProposalData(address[] memory targets, bytes[] memory calldatas, uint8 proposalTypeId) external;
}
8 changes: 2 additions & 6 deletions src/Validator.sol → src/lib/Validator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,9 @@ library Validator {
if (comparison == IProposalTypesConfigurator.Comparators.EQUAL) {
if (paramA != paramB) revert InvalidParamNotEqual();
} else if (comparison == IProposalTypesConfigurator.Comparators.LESS_THAN) {
if (paramA >= paramB) {
revert InvalidParamRange();
}
if (paramA >= paramB) revert InvalidParamRange();
} else if (comparison == IProposalTypesConfigurator.Comparators.GREATER_THAN) {
if (paramA <= paramB) {
revert InvalidParamRange();
}
if (paramA <= paramB) revert InvalidParamRange();
} else {
revert InvalidComparison();
}
Expand Down
26 changes: 12 additions & 14 deletions src/modules/ApprovalVotingModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ struct ProposalOption {
}

struct Proposal {
address governor;
uint256 initBalance;
uint128[] optionVotes;
ProposalOption[] options;
Expand Down Expand Up @@ -92,13 +91,16 @@ contract ApprovalVotingModule is VotingModule {
* @param proposalId The id of the proposal.
* @param proposalData The proposal data encoded as `PROPOSAL_DATA_ENCODING`.
*/
function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash) external override {
_onlyGovernor();
if (proposalId != uint256(keccak256(abi.encode(msg.sender, address(this), proposalData, descriptionHash)))) {
function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash)
external
override
onlyGovernor
{
if (proposalId != uint256(keccak256(abi.encode(governor, address(this), proposalData, descriptionHash)))) {
revert WrongProposalId();
}

if (proposals[proposalId].governor != address(0)) {
if (proposals[proposalId].optionVotes.length != 0) {
revert ExistingProposal();
}

Expand Down Expand Up @@ -128,7 +130,6 @@ contract ApprovalVotingModule is VotingModule {
}
}

proposals[proposalId].governor = msg.sender;
proposals[proposalId].settings = proposalSettings;
proposals[proposalId].optionVotes = new uint128[](optionsLength);
}
Expand All @@ -146,8 +147,8 @@ contract ApprovalVotingModule is VotingModule {
external
virtual
override
onlyGovernor
{
_onlyGovernor();
Proposal memory proposal = proposals[proposalId];

if (support == uint8(VoteType.For)) {
Expand Down Expand Up @@ -175,19 +176,18 @@ contract ApprovalVotingModule is VotingModule {
function _formatExecuteParams(uint256 proposalId, bytes memory proposalData)
public
override
onlyGovernor
returns (address[] memory targets, uint256[] memory values, bytes[] memory calldatas)
{
_onlyGovernor();
(ProposalOption[] memory options, ProposalSettings memory settings) =
abi.decode(proposalData, (ProposalOption[], ProposalSettings));

{
IAgoraGovernor governor = IAgoraGovernor(proposals[proposalId].governor);

// If budgetToken is not ETH
if (settings.budgetToken != address(0)) {
// Save initBalance to be used as comparison in `_afterExecute`
proposals[proposalId].initBalance = IERC20(settings.budgetToken).balanceOf(governor.timelock());
proposals[proposalId].initBalance =
IERC20(settings.budgetToken).balanceOf(IAgoraGovernor(governor).timelock());
}
}

Expand Down Expand Up @@ -286,10 +286,8 @@ contract ApprovalVotingModule is VotingModule {
(, ProposalSettings memory settings) = abi.decode(proposalData, (ProposalOption[], ProposalSettings));

if (settings.budgetToken != address(0) && settings.budgetAmount > 0) {
IAgoraGovernor governor = IAgoraGovernor(proposals[proposalId].governor);

uint256 initBalance = proposals[proposalId].initBalance;
uint256 finalBalance = IERC20(settings.budgetToken).balanceOf(governor.timelock());
uint256 finalBalance = IERC20(settings.budgetToken).balanceOf(IAgoraGovernor(governor).timelock());

// If `finalBalance` is higher than `initBalance`, ignore the budget check
if (finalBalance < initBalance) {
Expand Down
23 changes: 12 additions & 11 deletions src/modules/OptimisticModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ struct ProposalSettings {
}

struct Proposal {
address governor;
ProposalSettings settings;
}

Expand Down Expand Up @@ -60,21 +59,24 @@ contract OptimisticModule is VotingModule {
* @param proposalId The id of the proposal.
* @param proposalData The proposal data encoded as `PROPOSAL_DATA_ENCODING`.
*/
function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash) external override {
_onlyGovernor();
if (proposalId != uint256(keccak256(abi.encode(msg.sender, address(this), proposalData, descriptionHash)))) {
function propose(uint256 proposalId, bytes memory proposalData, bytes32 descriptionHash)
external
override
onlyGovernor
{
if (proposalId != uint256(keccak256(abi.encode(governor, address(this), proposalData, descriptionHash)))) {
revert WrongProposalId();
}

if (proposals[proposalId].governor != address(0)) {
if (proposals[proposalId].settings.againstThreshold != 0) {
revert ExistingProposal();
}

ProposalSettings memory proposalSettings = abi.decode(proposalData, (ProposalSettings));

uint8 proposalTypeId = IAgoraGovernor(msg.sender).getProposalType(proposalId);
uint8 proposalTypeId = IAgoraGovernor(governor).getProposalType(proposalId);
IProposalTypesConfigurator proposalConfigurator =
IProposalTypesConfigurator(IAgoraGovernor(msg.sender).PROPOSAL_TYPES_CONFIGURATOR());
IProposalTypesConfigurator(IAgoraGovernor(governor).PROPOSAL_TYPES_CONFIGURATOR());
IProposalTypesConfigurator.ProposalType memory proposalType = proposalConfigurator.proposalTypes(proposalTypeId);

if (proposalType.quorum != 0 || proposalType.approvalThreshold != 0) {
Expand All @@ -87,7 +89,6 @@ contract OptimisticModule is VotingModule {
revert InvalidParams();
}

proposals[proposalId].governor = msg.sender;
proposals[proposalId].settings = proposalSettings;
}

Expand Down Expand Up @@ -127,12 +128,12 @@ contract OptimisticModule is VotingModule {
*/
function _voteSucceeded(uint256 proposalId) external view override returns (bool) {
Proposal memory proposal = proposals[proposalId];
(uint256 againstVotes,,) = IAgoraGovernor(proposal.governor).proposalVotes(proposalId);
(uint256 againstVotes,,) = IAgoraGovernor(governor).proposalVotes(proposalId);

uint256 againstThreshold = proposal.settings.againstThreshold;
if (proposal.settings.isRelativeToVotableSupply) {
uint256 snapshotBlock = IGovernorUpgradeable(proposal.governor).proposalSnapshot(proposalId);
IVotesUpgradeable token = IAgoraGovernor(proposal.governor).token();
uint256 snapshotBlock = IGovernorUpgradeable(governor).proposalSnapshot(proposalId);
IVotesUpgradeable token = IAgoraGovernor(governor).token();
againstThreshold = (token.getPastTotalSupply(snapshotBlock) * againstThreshold) / PERCENT_DIVISOR;
}

Expand Down
Loading

0 comments on commit aa946e1

Please sign in to comment.