Olympus DAO contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L205-L262

Vulnerability details

activateProposal() need time delay

Impact

There is no time lock or delay when activating a proposal, the previous one could be replaced immediately. In vote() call, a user might want to vote for the previous proposal, but if the vote() call and the activateProposal() is very close or even in the same block, it is quite possible that the user actually voted for another proposal without much knowledge of. A worse case is some malicious user watching the mempool, and front run a big vote favor/against the activeProposal, effectively influence the voting result.

These situations are not what the governance intends to deliver, and might also affect the results of 2 proposals.

Proof of Concept

activateProposal() can take effect right away, replacing the activeProposal. And vote() does not specify which proposalId to vote for, but the activeProposal could be different from last second.

src/policies/Governance.sol

    function activateProposal(uint256 proposalId_) external {
        ProposalMetadata memory proposal = getProposalMetadata[proposalId_];

        if (msg.sender != proposal.submitter) {
            revert NotAuthorizedToActivateProposal();
        }

        if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
            revert SubmittedProposalHasExpired();
        }

        if (
            (totalEndorsementsForProposal[proposalId_] * 100) <
            VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }

        if (proposalHasBeenActivated[proposalId_] == true) {
            revert ProposalAlreadyActivated();
        }

        if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
            revert ActiveProposalNotExpired();
        }

        activeProposal = ActivatedProposal(proposalId_, block.timestamp);

        proposalHasBeenActivated[proposalId_] = true;

        emit ProposalActivated(proposalId_, block.timestamp);
    }

    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Add time delay when activating a proposal, so that users can be aware of that and vote for the current one within the time window.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L268-L270

Vulnerability details

Impact

Even if all the users do nothing, _mint() or _burn() of OlympusVotes tokens could potentially change the result of the voting. Which might cause confusion to some users, or this pattern could be used to manipulate the result of governance voting. Such as combined with a DoS griefing using submitProposal() with very long instructions_ array. Or someone could monitor the mempool, waiting for a big amount of _mint() or _burn() in favor/against of the current vote, and execute or replace with another activation.

The influence of total supply of OlympusVotes tokens might be used to affect the voting functioning, causing unexpected governance functioning. But it could be mitigated simply through set certain time period for these operations.

Proof of Concept

src/policies/Governance.sol

        if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
            revert NotEnoughVotesToExecute();
        }

In the situation netVotes is on the edge of threshold, a call to _mint() or _burn() of OlympusVotes tokens can change the result of the vote.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Reserve some grace period between 2 different voting, for _mint() and _burn() OlympusVotes tokens. And check if some proposal is in voting in _mint() and _burn():

    if (gov.activeProposal.proposalId != 0) revert;

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L161-L174

Vulnerability details

Impact

latestRoundData() is used when trying to get currentPrice, but there is no check if the return value indicates stale data and round completeness. There are no checks on roundID might result in stale prices. The oracle wrapper calls out to a chainlink oracle receiving the latestRoundData().

If there is a problem with chainlink starting a new round and finding consensus on the new value for the oracle (e.g. chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the chainlink system) consumers of this contract may continue using outdated stale data (if oracles are unable to submit no new round is started).

This could lead to stale prices and can lead to wrong currentPrice return value.

Reference to the Chainlink documentation: https://docs.chain.link/docs/historical-price-data/#historical-rounds

Proof of Concept

src/modules/PRICE.sol

161-173:
        (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
        if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
            revert Price_BadFeed(address(_ohmEthPriceFeed));
        ohmEthPrice = uint256(ohmEthPriceInt);

        int256 reserveEthPriceInt;
        (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
        if (updatedAt < block.timestamp - uint256(observationFrequency))
            revert Price_BadFeed(address(_reserveEthPriceFeed));
        reserveEthPrice = uint256(reserveEthPriceInt);

Stale prices could make the currentPrice inaccurate and affect the moving average price. According to Chainlink's documentation, This function does not error if no answer has been reached but returns 0, causing an incorrect price feed to the PriceOracle. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations of the AMM. Oracle reliance has historically resulted in crippled on-chain systems, and complications that lead to these outcomes can arise from things as simple as network congestion.

Although there is a timestamp check, Round ID is not checked in the function. That can also cause stale price.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Validate data feed on both Round ID and timestamp:

        (uint80 roundID, int256 ohmEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _ohmEthPriceFeed.latestRoundData();
        if (updatedAt < block.timestamp - 3 * uint256(observationFrequency) ||
        answeredInRound < roundID)
            revert Price_BadFeed(address(_ohmEthPriceFeed));
        ohmEthPrice = uint256(ohmEthPriceInt);

        int256 reserveEthPriceInt;
        (uint80 roundID, reserveEthPriceInt, , updatedAt, uint80 answeredInRound) = _reserveEthPriceFeed.latestRoundData();
        if (updatedAt < block.timestamp - uint256(observationFrequency) ||
        answeredInRound < roundID)
            revert Price_BadFeed(address(_reserveEthPriceFeed));
        reserveEthPrice = uint256(reserveEthPriceInt);

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

src/Kernel.sol
203-208:
    event PermissionsUpdated(
        Keycode indexed keycode_,
        Policy indexed policy_,
        bytes4 funcSelector_,
        bool granted_
    );

src/modules/PRICE.sol
26:
    event NewObservation(uint256 timestamp_, uint256 price_, uint256 movingAverage_);

src/modules/RANGE.sol
20-29:
    event WallUp(bool high_, uint256 timestamp_, uint256 capacity_);
    event WallDown(bool high_, uint256 timestamp_, uint256 capacity_);
    event CushionUp(bool high_, uint256 timestamp_, uint256 capacity_);

    event PricesChanged(
        uint256 wallLowPrice_,
        uint256 cushionLowPrice_,
        uint256 cushionHighPrice_,
        uint256 wallHighPrice_
    );

src/modules/TRSRY.sol
20:
    event ApprovedForWithdrawal(address indexed policy_, ERC20 indexed token_, uint256 amount_);
27-29:    
    event DebtIncurred(ERC20 indexed token_, address indexed policy_, uint256 amount_);
    event DebtRepaid(ERC20 indexed token_, address indexed policy_, uint256 amount_);
    event DebtSet(ERC20 indexed token_, address indexed policy_, uint256 amount_);

src/policies/Governance.sol
87:
    event ProposalEndorsed(uint256 proposalId, address voter, uint256 amount);
89:
    event WalletVoted(uint256 proposalId, address voter, bool for_, uint256 userVotes);

src/policies/Operator.sol
45-50:
    event Swap(
        ERC20 indexed tokenIn_,
        ERC20 indexed tokenOut_,
        uint256 amountIn_,
        uint256 amountOut_
    );
52:
    event CushionParamsChanged(uint32 duration_, uint32 debtBuffer_, uint32 depositInterval_);
54:
    event RegenParamsChanged(uint32 wait_, uint32 threshold_, uint32 observe_);

AVOID FLOATING PRAGMAS: THE VERSION SHOULD BE LOCKED

The pragma declared across the solution is 0.8.15. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.6) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

These 2 use >=0.8.0, consider changing to 0.8.15 to be the same as the others. src/policies/interfaces/IHeart.sol src/policies/interfaces/IOperator.sol

Check for proposal status before endorseProposal()

If a proposal is already active or expired, it does not make sense to endorse anymore, some later operations can also be skipped to save some gas.

Suggestion:

In endorseProposal(), add status check:

        if (proposalHasBeenActivated[proposalId_] == true) {
            revert ProposalAlreadyActivated();
        }

        if (block.timestamp > getProposalMetadata[proposalId_].submissionTimestamp + ACTIVATION_DEADLINE) {
            revert SubmittedProposalHasExpired();
        }

userVotes can be 0 checked

src/policies/Governance.sol

    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

If userVotes is 0, all the subsequent code can be skipped to save gas.

Suggestion: add

    require(userVotes != 0);

EVENTS NOT EMITTED FOR IMPORTANT STATE CHANGES

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

src/policies/Operator.sol

    function setBondContracts(IBondAuctioneer auctioneer_, IBondCallback callback_)
        external
        onlyRole("operator_admin")
    {
        if (address(auctioneer_) == address(0) || address(callback_) == address(0))
            revert Operator_InvalidParams();
        /// Set contracts
        auctioneer = auctioneer_;
        callback = callback_;
    }

    function toggleActive() external onlyRole("operator_admin") {
        /// Toggle active state
        active = !active;
    }

Suggestion: Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity.

Some wrong oracle data feed should be looked up and fixed

Sometimes the oracle might return bad data feeds. For example a stale oracle, or in volatile market, the data might be inaccurate. If these data stay in the records, the moving average can be inaccurate.

Suggestion: Add some method to change the inappropriate data, or delete outliers.

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

src/Kernel.sol
397:    for (uint256 i = 0; i < reqLength; ) {

src/utils/KernelUtils.sol
43:     for (uint256 i = 0; i < 5; ) {
58:     for (uint256 i = 0; i < 32; ) {

The demo of the loop gas comparison can be seen here.

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

src/modules/PRICE.sol
136:    _movingAverage += (currentPrice - earliestPrice) / numObs;
222:    total += startObservations_[i];

138:    _movingAverage -= (earliestPrice - currentPrice) / numObs;

src/modules/TRSRY.sol
96-97:
        reserveDebt[token_][msg.sender] += amount_;
        totalDebt[token_] += amount_;
131:
        if (oldDebt < amount_) totalDebt[token_] += amount_ - oldDebt;

115-116:
        reserveDebt[token_][msg.sender] -= received;
        totalDebt[token_] -= received;
132:
        else totalDebt[token_] -= oldDebt - amount_;

src/modules/VOTES.sol
58:     balanceOf[to_] += amount_;

56:     balanceOf[from_] -= amount_;


src/policies/BondCallback.sol
143-144:
        _amountsPerMarket[id_][0] += inputAmount_;
        _amountsPerMarket[id_][1] += outputAmount_;

src/policies/Governance.sol
194:    totalEndorsementsForProposal[proposalId_] -= previousEndorsement;
198:    totalEndorsementsForProposal[proposalId_] += userVotes;
252:    yesVotesForProposal[activeProposal.proposalId] += userVotes;
254:    noVotesForProposal[activeProposal.proposalId] += userVotes;

src/policies/Heart.sol
103:    lastBeat += frequency();

MULTIPLE MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN key TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

src/Kernel.sol: Keynode

167-181:
    mapping(Keycode => Module) public getModuleForKeycode;

    mapping(Keycode => Policy[]) public moduleDependents;

    mapping(Keycode => mapping(Policy => uint256)) public getDependentIndex;

    mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions;

src/modules/TRSRY.sol: ERC20

36-39:
    mapping(ERC20 => uint256) public totalDebt;

    mapping(ERC20 => mapping(address => uint256)) public reserveDebt;

src/policies/Governance.sol: id

96-117:
    mapping(uint256 => ProposalMetadata) public getProposalMetadata;

    mapping(uint256 => uint256) public totalEndorsementsForProposal;

    mapping(uint256 => mapping(address => uint256)) public userEndorsementsForProposal;

    mapping(uint256 => bool) public proposalHasBeenActivated;

    mapping(uint256 => uint256) public yesVotesForProposal;

    mapping(uint256 => uint256) public noVotesForProposal;

    mapping(uint256 => mapping(address => uint256)) public userVotesForProposal;

    mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;

MAKING SOME CONSTANTS AS NON-PUBLIC

Changing the visibility from public to private or internal can save gas when a constant isn’t used outside of its contract.

Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

If needed, the value can be read from the verified contract source code.

A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.

The followings can be changed from public to internal or private:

src/policies/Governance.sol:
121-137:
    uint256 public constant SUBMISSION_REQUIREMENT = 100;

    uint256 public constant ACTIVATION_DEADLINE = 2 weeks;

    uint256 public constant GRACE_PERIOD = 1 weeks;

    uint256 public constant ENDORSEMENT_THRESHOLD = 20;

    uint256 public constant EXECUTION_THRESHOLD = 33;

    uint256 public constant EXECUTION_TIMELOCK = 3 days;

src/policies/Operator.sol
89:     uint32 public constant FACTOR_SCALE = 1e4;

src/modules/RANGE.sol
65:     uint256 public constant FACTOR_SCALE = 1e4;

if condition checks can be performed earlier

Some if statement checks can be done earlier. If it reverts, the other operations can be saved.

1. store() in src/modules/INSTR.sol

if (length == 0) can be moved right after length is assigned.

43-48:
        uint256 length = instructions_.length;
        uint256 instructionsId = ++totalInstructions;

        Instruction[] storage instructions = storedInstructions[instructionsId];

        if (length == 0) revert INSTR_InstructionsCannotBeEmpty();

Suggestion: Change to:

        uint256 length = instructions_.length;
        if (length == 0) revert INSTR_InstructionsCannotBeEmpty();
        
        uint256 instructionsId = ++totalInstructions;

        Instruction[] storage instructions = storedInstructions[instructionsId];

2. endorseProposal() in src/policies/Governance.sol

The checks can be moved to the beginning.

181-190:
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

Suggestion: Change to:

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        uint256 userVotes = VOTES.balanceOf(msg.sender);

3. vote() in src/policies/Governance.sol

The checks can be moved to the beginning.

241-249;
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

executed instructions can be deleted and get gas refund

New instructions are saved by calling store() in src/modules/INSTR.sol, and executed by calling executeAction() in src/Kernel.sol. But after the execution, the storage space is not recycled.

Suggestion: After the execution, delete the instruction struct as well as the key in storedInstructions mapping.

Outdated proposals can be recycled

Some proposals are outdated and can be deleted to get some gas refund.

• expired
• executed
• not meet the endorsement threshold
• not meet the execution threshold
• missed the execution time

User endorsement for proposal accounting

To update a user's endorsement for a proposal, the previous votes were subtracted from the count. If the user is duplicating an endorsement, Each time it involves multiple storage manipulations which is costly. To save some gas, a mapping to record whether a user has endorsed can be maintained, and checked before written into storage. After the proposal is inactivated, the record can be deleted for some gas refund.

sload/sstore cost at least 100 gas (warm access), while memory operations mload/mstore cost only 3 gas.

Currently each time the endorsement 4 sstore operations. src/policies/Governance.sol

    function endorseProposal(uint256 proposalId_) external {
192-198：
        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

Suggestion:

• only write into totalEndorsementsForProposal the difference, the amount saved is near 100 gas.
• create isUserEndorsementsForProposal as the record. Only write into storage the difference of the endorsement.
• X = X + Y / X = X - Y costs less than X += Y / X -= Y

    mapping(uint256 => mapping(address => bool)) private isUserEndorsementsForProposal;

    function endorseProposal(uint256 proposalId_) external {

        if (!isUserEndorsementsForProposal[proposalId_][msg.sender])) {
                userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
                totalEndorsementsForProposal[proposalId_] = totalEndorsementsForProposal[proposalId_] + userVotes;

                isUserEndorsementsForProposal[proposalId_][msg.sender] = true;
        } else {
                uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
                if (previousEndorsement != userVotes) {
                        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
                        totalEndorsementsForProposal[proposalId_] = totalEndorsementsForProposal[proposalId_] - previousEndorsement + userVotes;
                }
        }

Variables referred multiple times can be cached in local memory

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference) Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

1. activeProposal.proposalId

src/policies/Governance.sol
240-289:
    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

    function executeProposal() external {
        uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
            noVotesForProposal[activeProposal.proposalId];
        if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
            revert NotEnoughVotesToExecute();
        }

        if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
            revert ExecutionTimelockStillActive();
        }

        Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId);

        for (uint256 step; step < instructions.length; ) {
            kernel.executeAction(instructions[step].action, instructions[step].target);
            unchecked {
                ++step;
            }
        }

        emit ProposalExecuted(activeProposal.proposalId);

        // deactivate the active proposal
        activeProposal = ActivatedProposal(0, 0);
    }

2. msg.sender

src/policies/Governance.sol
159-201:
    function submitProposal(
        Instruction[] calldata instructions_,
        bytes32 title_,
        string memory proposalURI_
    ) external {
        if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
            revert NotEnoughVotesToPropose();

        uint256 proposalId = INSTR.store(instructions_);
        getProposalMetadata[proposalId] = ProposalMetadata(
            title_,
            msg.sender,
            block.timestamp,
            proposalURI_
        );

        emit ProposalSubmitted(proposalId);
    }

    function endorseProposal(uint256 proposalId_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

240-313:
    function vote(bool for_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (activeProposal.proposalId == 0) {
            revert NoActiveProposalDetected();
        }

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

    function executeProposal() external {
        uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
            noVotesForProposal[activeProposal.proposalId];
        if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
            revert NotEnoughVotesToExecute();
        }

        if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
            revert ExecutionTimelockStillActive();
        }

        Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId);

        for (uint256 step; step < instructions.length; ) {
            kernel.executeAction(instructions[step].action, instructions[step].target);
            unchecked {
                ++step;
            }
        }

        emit ProposalExecuted(activeProposal.proposalId);

        // deactivate the active proposal
        activeProposal = ActivatedProposal(0, 0);
    }

    function reclaimVotes(uint256 proposalId_) external {
        uint256 userVotes = userVotesForProposal[proposalId_][msg.sender];

        if (userVotes == 0) {
            revert CannotReclaimZeroVotes();
        }

        if (proposalId_ == activeProposal.proposalId) {
            revert CannotReclaimTokensForActiveVote();
        }

        if (tokenClaimsForProposal[proposalId_][msg.sender] == true) {
            revert VotingTokensAlreadyReclaimed();
        }

        tokenClaimsForProposal[proposalId_][msg.sender] = true;

        VOTES.transferFrom(address(this), msg.sender, userVotes);
    }

3. frequency()

    function beat() external nonReentrant {
        // ...
        if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();

        // ...
        lastBeat += frequency();

        // ...
    }

BOOLEAN COMPARISON

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

Suggestion: Use if (value) instead of if (value == true)

in the following:

src/policies/Governance.sol
223:    if (proposalHasBeenActivated[proposalId_] == true) {

306:    if (tokenClaimsForProposal[proposalId_][msg.sender] == true) {

MAKING SOME VARIABLES AS NON-PUBLIC

Changing the visibility from public to private or internal. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

src/policies/Governance.sol

    ActivatedProposal public activeProposal;

Variable re-arrangement by storage packing

In src/policies/Heart.sol, bool public active can be placed next to ERC20 public rewardToken, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

32-42:
    /// @notice Status of the Heart, false = stopped, true = beating
    bool public active;

    /// @notice Timestamp of the last beat (UTC, in seconds)
    uint256 public lastBeat;

    /// @notice Reward for beating the Heart (in reward token decimals)
    uint256 public reward;

    /// @notice Reward token address that users are sent for beating the Heart
    ERC20 public rewardToken;

Reference: Layout of State Variables in Storage.

X = X + 1 / X = X - 1 IS CHEAPER THAN X += 1 / X++ / ++X / X -= 1 / X-- / --X

The demo of the gas comparison can be seen here.

Consider use unchecked{ X = X + 1 } to save gas.

src/policies/Operator.sol

488:        decimals++;
670:        _status.low.count++;
686:        _status.high.count++;

675:        _status.low.count--;
691:        _status.high.count--;

Duplicate call in operate()

1. PRICE.getMovingAverage()

In operate(), _updateRangePrices() and _addObservation() both call PRICE.getMovingAverage().

src/policies/Operator.sol

    function operate() external override onlyWhileActive onlyRole("operator_operate") {
        // ...
        /// Update the prices for the range, save new regen observations, and update capacities based on bond market activity
        _updateRangePrices();
        _addObservation();
        // ...
    }

    function _updateRangePrices() internal {
        /// Get latest moving average from the price module
        uint256 movingAverage = PRICE.getMovingAverage();

        /// Update the prices on the range module
        RANGE.updatePrices(movingAverage);
    }

    function _addObservation() internal {
        /// Get latest moving average from the price module
        uint256 movingAverage = PRICE.getMovingAverage();
        // ...
    }

Suggestion:

Pass movingAverage as an argument to these 2 functions.

    function operate() external override onlyWhileActive onlyRole("operator_operate") {
        // ...
        /// Update the prices for the range, save new regen observations, and update capacities based on bond market activity
        uint256 movingAverage = PRICE.getMovingAverage();
        _updateRangePrices(movingAverage);
        _addObservation(movingAverage);
        // ...
    }

    function _updateRangePrices(uint movingAverage) internal {
        /// Update the prices on the range module
        RANGE.updatePrices(movingAverage);
    }

    function _addObservation(uint movingAverage) internal {
        // ...
    }

2. PRICE.getLastPrice()

In operate(), _addObservation() and operate() itself both call PRICE.getLastPrice().

src/policies/Operator.sol

    function operate() external override onlyWhileActive onlyRole("operator_operate") {
        // ...
        _addObservation();
        // ...
227:    uint256 currentPrice = PRICE.getLastPrice();
        // ...
    }

    function _addObservation() internal {
        // ...
659:    uint256 currentPrice = PRICE.getLastPrice();
        // ...
    }

Suggestion:

Pass currentPrice as an argument to _addObservation().

    function operate() external override onlyWhileActive onlyRole("operator_operate") {
        // ...
227:    uint256 currentPrice = PRICE.getLastPrice();
        // ...
        _addObservation(currentPrice);
        // ...
    }

    function _addObservation(uint currentPrice) internal {
        // ...
    }

Some repeating calculations can be stored into an immutable

1. fullCapacity() Since fullCapacity() is called by _regenerate in operate(), it is a frequently used function. There is no need to redo the calculation every time.

src/policies/Operator.sol

    function fullCapacity(bool high_) public view override returns (uint256) {
        uint256 reservesInTreasury = TRSRY.getReserveBalance(reserve);
        uint256 capacity = (reservesInTreasury * _config.reserveFactor) / FACTOR_SCALE;
        if (high_) {
            capacity =
                (capacity.mulDiv(
                    10**ohmDecimals * 10**PRICE.decimals(),
                    10**reserveDecimals * RANGE.price(true, true)
                ) * (FACTOR_SCALE + RANGE.spread(true) * 2)) /
                FACTOR_SCALE;
        }
        return capacity;
    }

Suggestion: Precalculate and save the following when initialize the contract:

        10**ohmDecimals * 10**PRICE.decimals() / 10**reserveDecimals / FACTOR_SCALE;

2. getAmountOut()

752-754:
        uint256 amountOut = amountIn_.mulDiv(
            10**reserveDecimals * RANGE.price(true, false),
            10**ohmDecimals * 10**PRICE.decimals()
763-765:
        uint256 amountOut = amountIn_.mulDiv(
            10**ohmDecimals * 10**PRICE.decimals(),
            10**reserveDecimals * RANGE.price(true, true)

Suggestion: Precalculate and save the following when initialize the contract:

752-754:
        10**reserveDecimals / 10**ohmDecimals / 10**PRICE.decimals()
763-765:
        10**ohmDecimals * 10**PRICE.decimals() / 10**reserveDecimals

Use bit shift instead of power operation

fullCapacity() is called by _regenerate in operate(), it is a frequently used function. _activate() is also called every time a bond market is deployed.

src/policies/Operator.sol

    function _activate(bool high_) internal {
            // ...
372:
            int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2);
            // ...
419-420:            
            uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
            uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;
            // ...
427:
            int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals / 2);
            // ...
    }

    function fullCapacity(bool high_) public view override returns (uint256) {
786:
                ) * (FACTOR_SCALE + RANGE.spread(true) * 2)) /
    }

Suggestion: Change the above to

    function _activate(bool high_) internal {
            // ...
372:
            int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals >> 1);
            // ...
419-420:            
            uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
            uint256 invWallPrice = 10**(oracleDecimals << 1) / range.wall.low.price;
            // ...
427:
            int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals >> 1);
            // ...
    }

    function fullCapacity(bool high_) public view override returns (uint256) {
786:
                ) * (FACTOR_SCALE + RANGE.spread(true) << 1)) /
    }

The demo of the gas comparison can be seen here.

PRICE.decimals() can be saved into a variable

PRICE.decimals() is used every time _activate(), _getPriceDecimals and getAmountOut() are called, since in module PRICE it is a constant, it will only be changed when the module is upgraded. It might be more gas efficient to save PRICE.decimals() into a immutable or at least a local variable, instead of calling the getter function.

src/policies/Operator.sol

    function _activate(bool high_) internal {
        // ...
375:
            uint256 oracleScale = 10**uint8(int8(PRICE.decimals()) - priceDecimals);

        // ...
418-420:
            uint8 oracleDecimals = PRICE.decimals();
            uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
            uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;
430:
            uint256 oracleScale = 10**uint8(int8(oracleDecimals) - priceDecimals);

Suggestion: save PRICE.decimals() into a immutable or a local variable.

function regenerate() never used in Operator.sol

    function regenerate(bool high_) external onlyRole("operator_admin") {
        /// Regenerate side
        _regenerate(high_);
    }

Suggestion: Remove function regenerate().