Olympus DAO contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L9-L11 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L205-L236

Vulnerability details

Impact

The following comment indicates that the OlympusVotes contract is a stub for gOHM. Checking the gOHM contract at https://etherscan.io/token/0x0ab87046fBb341D058F17CBC4c1133F25a20a52f#code, the transfer and transferFrom functions are available.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L9-L11

/// @notice Votes module is the ERC20 token that represents voting power in the network.
/// @dev    This is currently a substitute module that stubs gOHM.
contract OlympusVotes is Module, ERC20 {

Moreover, the documentation states that the vote redemption mechanism "exists to deter malicious behavior by ensuring users cannot transfer their voting tokens until after the proposal has been resolved", which also indicates that the voting tokens are meant to be transferrable between users.

When the voting tokens are transferrable, one user can first use her or his votes to call the following endorseProposal function to endorse a proposal and then transfer these votes to another user. The other user can use these votes to endorse the same proposal again afterwards. Because of the double-endorsement, the (totalEndorsementsForProposal[proposalId_] * 100) < VOTES.totalSupply() * ENDORSEMENT_THRESHOLD condition can become true so the proposal can be activated by calling the activateProposal function below. However, the proposal should only be endorsed with these same votes once and should not be able to be activated if it could not satisify (totalEndorsementsForProposal[proposalId_] * 100) < VOTES.totalSupply() * ENDORSEMENT_THRESHOLD with these votes being used once.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

    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);
    }

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

    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);
    }

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_UserEndorsesAfterReceivingTransferredVotes() public {
        _submitProposal();

        vm.prank(voter2);
        governance.endorseProposal(1);

        // to simulate calling gOHM's transfer function by voter2 for sending votes to voter0, VOTES.transferFrom is called by governance here
        vm.prank(address(governance));
        VOTES.transferFrom(voter2, voter0, 200);

        // voter0 uses the votes previously owned by voter2 to endorse the proposal
        vm.prank(voter0);
        governance.endorseProposal(1);

        // the proposal is endorsed with 400 votes but only the 200 votes originally owned by voter2 are used
        assertEq(governance.userEndorsementsForProposal(1, voter0), 200);
        assertEq(governance.userEndorsementsForProposal(1, voter2), 200);
        assertEq(governance.totalEndorsementsForProposal(1), 400);

        // At this moment, the proposal can be activated successfully.
        // However, if it is endorsed with only 200 votes, it cannot satisfy ENDORSEMENT_THRESHOLD and cannot be activated.
        vm.expectEmit(true, true, true, true);
        emit ProposalActivated(1, block.timestamp);

        vm.prank(voter1);
        governance.activateProposal(1);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When calling endorseProposal, the user's votes can be locked by transferring these votes to the governance so the user cannot transfer these anymore to another user after the endorsement. An additional function can be added for reclaiming the endorsed votes back to the user and reducing the proposal's endorsed votes accordingly before the proposal is activated. After the proposal is activated, the endorsed votes should be counted as the voted votes.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

Vulnerability details

Impact

The voter admin can call the following revokeVotesFrom function to revoke a user's votes, which also decreases the total supply of the votes, after the user endorses a proposal through calling the endorseProposal function below. Because endorseProposal can be called multiple times, the user has the incentive to call it for endorsing the proposal again with the new votes minted by the issueVotesTo function. However, after the user's votes are revoked, the user has no incentive to call endorseProposal again. Hence, the endorsed votes by the user for the proposal does not decrease after the user's votes are revoked. When determining whether the proposal can be activated or not, its old endorsed votes, which is not decreased, are compared against the new total supply of the votes, which is decreased because of the revokeVotesFrom call. As a result, the proposal is unreliably more likely to satisfy the condition for being activated.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201

    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);
    }

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_EndorsedVotesDoNotDecreaseAfterVotesAreRevoked() public {
        _submitProposal();

        // voter3 endorse the proposal
        vm.prank(voter3);
        governance.endorseProposal(1);

        assertEq(governance.userEndorsementsForProposal(1, voter3), 300);
        assertEq(governance.totalEndorsementsForProposal(1), 300);

        // to simulate calling VoterRegistration.revokeVotesFrom that burns voter3's votes, VOTES.burnFrom is called by godmode here
        vm.prank(godmode);
        VOTES.burnFrom(voter3, 300);

        // at this moment, voter3 has 0 votes
        assertEq(VOTES.balanceOf(voter3), 0);

        // however, the proposal is still endorsed with voter3's previous votes
        assertEq(governance.userEndorsementsForProposal(1, voter3), 300);
        assertEq(governance.totalEndorsementsForProposal(1), 300);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When revokeVotesFrom is called during the time for endorsement, the corresponding votes that are previously endorsed for a proposal and are now revoked should be removed from the proposal's endorsed votes for the user. This ensures that the endorsed votes and the votes' total supply after the revocation are in sync for the proposal.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240-L262 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

Vulnerability details

Impact

A user can call the following vote function to vote for a proposal. During voting, the voter admin can still call the issueVotesTo and revokeVotesFrom functions below to issue new votes or revoke old votes for the user, which also changes the votes' total supply during the overall voting. Because each user can only call vote once for a proposal due to the userVotesForProposal[activeProposal.proposalId][msg.sender] > 0 conditional check, the old voted votes, resulted from the vote call by the user, will be used to compare against the new total supply of the votes, resulted from the issueVotesTo and revokeVotesFrom calls during the overall voting, when determining whether the proposal can be executed or not. Because of this inconsistency, the result on whether the proposal can be executed might not be reliable.

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

    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);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48

    function issueVotesTo(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Issue the votes in the VOTES module
        VOTES.mintTo(wallet_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

Proof of Concept

Please add the following code in src\test\policies\Governance.t.sol.

First, please add the following code for stdError.

import {Test, stdError} from "forge-std/Test.sol";    // @audit import stdError for testing purpose

Then, please append the following tests. These tests will pass to demonstrate the described scenarios.

    function testScenario_UserCannotVoteAgainWithNewlyMintedVotes() public {
        _createActiveProposal();

        // voter3 votes for the proposal
        vm.prank(voter3);
        governance.vote(true);

        assertEq(governance.yesVotesForProposal(1), 300);
        assertEq(governance.noVotesForProposal(1), 0);

        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);

        // to simulate calling VoterRegistration.issueVotesTo that mints votes to voter3, VOTES.mintTo is called by godmode here
        vm.prank(godmode);
        VOTES.mintTo(voter3, 500);
        assertEq(VOTES.balanceOf(voter3), 500);

        // calling vote function again by voter3 reverts, which means that voter3 cannot additionally vote with the 500 newly minted votes
        vm.expectRevert(UserAlreadyVoted.selector);
        vm.prank(voter3);
        governance.vote(true);
    }

    function testScenario_RevokeVotesAfterUserFinishsOwnVoting() public {
        _createActiveProposal();

        // voter3 votes for the proposal
        vm.prank(voter3);
        governance.vote(true);

        assertEq(governance.yesVotesForProposal(1), 300);
        assertEq(governance.noVotesForProposal(1), 0);

        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);

        // To simulate calling VoterRegistration.revokeVotesFrom that burns voter3's votes, VOTES.burnFrom is called by godmode here.
        // However, calling VOTES.burnFrom will revert due to arithmetic underflow.
        vm.prank(godmode);
        vm.expectRevert(stdError.arithmeticError);
        VOTES.burnFrom(voter3, 300);

        // the proposal is still voted with voter3's previous votes afterwards
        assertEq(governance.userVotesForProposal(1, voter3), 300);
        assertEq(VOTES.balanceOf(voter3), 0);
        assertEq(VOTES.balanceOf(address(governance)), 300);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When issueVotesTo and revokeVotesFrom are called during voting, the corresponding votes need to be added to or removed from the proposal's voted votes for the user. Alternatively, issueVotesTo and revokeVotesFrom can be disabled when an active proposal exists.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

Vulnerability details

Impact

During voting, the voter admin can call the following issueVotesTo and revokeVotesFrom functions to issue new votes or revoke old votes for any users. When the proposal can be executed based on the current votes that are in, the voter admin can revoke all other votes that are not yet in to support the proposal or mint many votes to a colluded user so that user can vote against the proposal. The vice versa is also true. Hence, the voting results can always be manipulated to favor the voter admin.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L45-L48

    function issueVotesTo(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Issue the votes in the VOTES module
        VOTES.mintTo(wallet_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/VoterRegistration.sol#L53-L56

    function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
        // Revoke the votes in the VOTES module
        VOTES.burnFrom(wallet_, amount_);
    }

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_VoterAdminCanManipulateVotingByIssuingOrRevokingVotes() public {
        _createActiveProposal();

        vm.prank(voter1);
        governance.vote(false);

        vm.prank(voter2);
        governance.vote(true);

        vm.prank(voter3);
        governance.vote(true);

        vm.prank(voter4);
        governance.vote(true);

        // At this moment, voter5 has not voted yet.
        // If voter5 votes no, this proposal cannot be activated; otherwise, this proposal can be activated.
        assertEq(governance.yesVotesForProposal(1), 900);
        assertEq(governance.noVotesForProposal(1), 100);

        // to simulate calling VoterRegistration.revokeVotesFrom that burns voter5's votes, VOTES.burnFrom is called by godmode here
        vm.prank(godmode);
        VOTES.burnFrom(voter5, 500);

        // after the voter admin burns voter5's votes, voter5 has no votes, and the proposal can only be activated because there are no more votes
        assertEq(VOTES.balanceOf(voter5), 0);
        assertEq(governance.yesVotesForProposal(1), 900);
        assertEq(governance.noVotesForProposal(1), 100);

        // furthermore, to simulate calling VoterRegistration.issueVotesTo that mints votes to voter0, VOTES.mintTo is called by godmode here
        vm.prank(godmode);
        VOTES.mintTo(voter0, 10000);
        assertEq(VOTES.balanceOf(voter0), 10000);

        // after the voter Admin mints 10000 votes to voter0, voter0 can vote yes for this proposal, and the proposal cannot be activated because most of the votes are no
        vm.prank(voter0);
        governance.vote(false);
        assertEq(governance.yesVotesForProposal(1), 900);
        assertEq(governance.noVotesForProposal(1), 10100);
    }

Tools Used

VSCode

Recommended Mitigation Steps

issueVotesTo and revokeVotesFrom can be disabled when an active proposal exists. If this is not possible, the risk of voting manipulation by the voter admin should be documented for raising users' awareness.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L154-L180

Vulnerability details

Impact

The current implementation compares updatedAt returned by the Chainlink oracle data feed's latestRoundData function against some arbitrary time windows of acceptance to determine if the returned answers are valid or not. However, these checks are not enough for preventing stale answers from being used.

According to the Chainlink's documentation:

1. roundId and answeredInRound are also returned. "You can check answeredInRound against the current roundId. If answeredInRound is less than roundId, the answer is being carried over. If answeredInRound is equal to roundId, then the answer is fresh."
2. "A read can revert if the caller is requesting the details of a round that was invalid or has not yet been answered. If you are deriving a round ID without having observed it before, the round might not be complete. To check the round, validate that the timestamp on that round is not 0."

Proof of Concept

When calling the following getCurrentPrice function, updatedAt returned by the latestRoundData function is used to check the updatedAt < block.timestamp - 3 * uint256(observationFrequency) condition for the validity of ohmEthPriceInt and the updatedAt < block.timestamp - uint256(observationFrequency) condition for the validity of reserveEthPriceInt. These arbitrary 3 * uint256(observationFrequency) and uint256(observationFrequency) time windows are used.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L154-L180

    function getCurrentPrice() public view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();

        // Get prices from feeds
        uint256 ohmEthPrice;
        uint256 reserveEthPrice;
        {
            (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
            // Use a multiple of observation frequency to determine what is too old to use.
            // Price feeds will not provide an updated answer if the data doesn't change much.
            // This would be similar to if the feed just stopped updating; therefore, we need a cutoff.
            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);
        }

        // Convert to OHM/RESERVE price
        uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice;

        return currentPrice;
    }

Yet, as indicated by the Chainlink's documentation, as mentioned in the Impact section above, answeredInRound >= roundId needs to be true for ensuring that the returned answer is not stale, and updatedAt != 0 needs to be true for ensuring that the round is complete. The current implementation's checks against the arbitrary time windows are more restrictive than updatedAt != 0 so the round completeness is verified. However, stale answers can still be returned and used because answeredInRound >= roundId is not currently enforced.

Tools Used

VSCode

Recommended Mitigation Steps

The getCurrentPrice function can be changed as follows.

1. answeredInRound and roundId can be returned by latestRoundData like the following.

(uint80 roundId, int256 ohmEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _ohmEthPriceFeed.latestRoundData()

2. The following require statement can be added for verifying whether the returned answer is stale or not.

require(answeredInRound >= roundId, "answer is stale");

3. In addition, the returned answer can also be verified as follows.

require(ohmEthPriceInt > 0, “answer is zero”);

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L122-L147

Vulnerability details

Impact

According to the following updateMovingAverage function, the change of moving average is calculated as either (currentPrice - earliestPrice) / numObs or (earliestPrice - currentPrice) / numObs. When currentPrice and earliestPrice are close such that currentPrice - earliestPrice or earliestPrice - currentPrice is less than numObs and is bigger than 0, the change of moving average will be calculated to be 0 because Solidity rounds down the result to the nearest integer. This makes the updated moving average unchanged but it actually should be increased or decreased by a non-zero amount.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L122-L147

    function updateMovingAverage() external permissioned {
        // Revert if not initialized
        if (!initialized) revert Price_NotInitialized();

        // Cache numbe of observations to save gas.
        uint32 numObs = numObservations;

        // Get earliest observation in window
        uint256 earliestPrice = observations[nextObsIndex];

        uint256 currentPrice = getCurrentPrice();

        // Calculate new moving average
        if (currentPrice > earliestPrice) {
            _movingAverage += (currentPrice - earliestPrice) / numObs;
        } else {
            _movingAverage -= (earliestPrice - currentPrice) / numObs;
        }

        // Push new observation into storage and store timestamp taken at
        observations[nextObsIndex] = currentPrice;
        lastObservationTime = uint48(block.timestamp);
        nextObsIndex = (nextObsIndex + 1) % numObs;

        emit NewObservation(block.timestamp, currentPrice, _movingAverage);
    }

Proof of Concept

Please append the following test in src\test\policies\PriceConfig.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_MovingAverageChangeCanBeZeroIfCurrentPriceAndEarliestPriceAreClose() public {
        // the following code lines perform the calculations used in the OlympusPrice.updateMovingAverage function

        uint256[] memory obs = getObs(0);
        uint256 numObs = obs.length;
        uint256 earliestPrice = obs[0];

        // if currentPrice - earliestPrice is less than numObs, the moving average change is 0
        uint256 currentPrice = earliestPrice + numObs - 1;
        uint256 _movingAverageChange = (currentPrice - earliestPrice) / numObs;
        assertEq(_movingAverageChange, 0);

        // if earliestPrice - currentPrice is less than numObs, the moving average change is also 0
        currentPrice = earliestPrice - numObs + 1;
        _movingAverageChange = (earliestPrice - currentPrice) / numObs;
        assertEq(_movingAverageChange, 0);
    }

Tools Used

VSCode

Recommended Mitigation Steps

To avoid iterating through observations for calculating the moving average, the total corresponding to the moving average can be stored in addition. When the moving average needs to be changed, earliestPrice can be subtracted from the total and currentPrice can be added to the total; the new total can then be divided by numObservations to get the updated moving average.

[L-01] IT IS POSSIBLE THAT beat FUNCTION IS NOT CALLED FOR ENTIRE TIME DURING A FREQUENCY

It is possible that the following beat function is not called by anybody for the entire time during a frequency. In this case, PRICE.updateMovingAverage() is not executed for that frequency. The price information for that frequency is not recorded, and the moving average becomes out-of-date as it is not updated with that frequency's price. Later, after someone calls beat again during a new frequency, the price information for the skipped frequency is still missing, and the duration between the current and earliest observations will be larger than specified. Because of this, the moving average deviates from the time-weighted average price to be more like an observation-weighted average price, which is also not as specified. To avoid these bookkeeping discrepancies, it can be beneficial to set up a bot to call beat for once during each frequency just in case nobody calls it during a frequency.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Heart.sol#L92-L109

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

        // Update the moving average on the Price module
        PRICE.updateMovingAverage();

        // Trigger price range update and market operations
        _operator.operate();

        // Update the last beat timestamp
        lastBeat += frequency();

        // Issue reward to sender
        _issueReward(msg.sender);

        emit Beat(block.timestamp);
    }

[L-02] UNRESOLVED TODO COMMENTS

Comment regarding todo indicates that there is an unresolved action item for implementation, which need to be addressed before protocol deployment. Please review the todo comments in the following code.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L657

        /// TODO determine if this should use the last price from the MA or recalculate the current price, ideally last price is ok since it should have been just updated and should include check against secondary?

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/TreasuryCustodian.sol#L51-L67

    // TODO Currently allows anyone to revoke any approval EXCEPT activated policies.
    // TODO must reorg policy storage to be able to check for deactivated policies.
    function revokePolicyApprovals(address policy_, ERC20[] memory tokens_) external {
        if (Policy(policy_).isActive()) revert PolicyStillActive();

        // TODO Make sure `policy_` is an actual policy and not a random address.

        uint256 len = tokens_.length;
        for (uint256 j; j < len; ) {
            TRSRY.setApprovalFor(policy_, tokens_[j], 0);
            unchecked {
                ++j;
            }
        }

        emit ApprovalRevoked(policy_, tokens_);
    }

[L-03] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESSES

To prevent unintended behaviors, the critical address inputs should be checked against address(0).

Please consider checking ohm_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/MINTR.sol#L15-L17

    constructor(Kernel kernel_, address ohm_) Module(kernel_) {
        ohm = OHM(ohm_);
    }

Please consider checking the addresses of ohmEthPriceFeed_ and reserveEthPriceFeed_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L71-L77

    constructor(
        Kernel kernel_,
        AggregatorV2V3Interface ohmEthPriceFeed_,
        AggregatorV2V3Interface reserveEthPriceFeed_,
        uint48 observationFrequency_,
        uint48 movingAverageDuration_
    ) Module(kernel_) {

Please consider checking the addresses in tokens_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/RANGE.sol#L77-L81

    constructor(
        Kernel kernel_,
        ERC20[2] memory tokens_,
        uint256[3] memory rangeParams_ // [thresholdFactor, cushionSpread, wallSpread]
    ) Module(kernel_) {

Please consider checking the addresses of aggregator_ and ohm_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/BondCallback.sol#L38-L45

    constructor(
        Kernel kernel_,
        IBondAggregator aggregator_,
        ERC20 ohm_
    ) Policy(kernel_) {
        aggregator = aggregator_;
        ohm = ohm_;
    }

Please consider checking the addresses in tokens_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L92-L98

    constructor(
        Kernel kernel_,
        IBondAuctioneer auctioneer_,
        IBondCallback callback_,
        ERC20[2] memory tokens_, // [ohm, reserve]
        uint32[8] memory configParams // [cushionFactor, cushionDuration, cushionDebtBuffer, cushionDepositInterval, reserveFactor, regenWait, regenThreshold, regenObserve]
    ) Policy(kernel_) {

Please consider checking the address of rewardToken_ in the following constructor. https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Heart.sol#L54-L59

    constructor(
        Kernel kernel_,
        IOperator operator_,
        ERC20 rewardToken_,
        uint256 reward_
    ) Policy(kernel_) {

[N-01] ESUBMISSION_REQUIREMENT IS USED TO COMPARE AGAINST 10000 BUT ENDORSEMENT_THRESHOLD AND EXECUTION_THRESHOLD ARE USED TO COMPARE AGAINST 100

ESUBMISSION_REQUIREMENT, ENDORSEMENT_THRESHOLD, and EXECUTION_THRESHOLD in the following code are all used to represent percents. However, ESUBMISSION_REQUIREMENT is used to compare against 10000 while ENDORSEMENT_THRESHOLD and EXECUTION_THRESHOLD are used to compare against 100. This inconsistency can cause confusions and typos in the future. Please consider unifying these constants so they can be used to compare against the same number.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L119-L133

    /// @notice The amount of votes a proposer needs in order to submit a proposal as a percentage of total supply (in basis points).
    /// @dev    This is set to 1% of the total supply.
    uint256 public constant SUBMISSION_REQUIREMENT = 100;

    ...

    /// @notice Endorsements required to activate a proposal as percentage of total supply.
    uint256 public constant ENDORSEMENT_THRESHOLD = 20;

    /// @notice Net votes required to execute a proposal on chain as a percentage of total supply.
    uint256 public constant EXECUTION_THRESHOLD = 33;

[N-02] Unreachable code

return true; is unreachable in the following code. It can be removed for better readability and maintainability.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L45-L48

    function transfer(address to_, uint256 amount_) public pure override returns (bool) {
        revert VOTES_TransferDisabled();
        return true;
    }

[N-03] decimals CAN BE NAMED USING CAPITAL LETTERS AND UNDERSCORES

Because the following decimals is a constant, it can be named using capital letters and underscores by convention, which improves readability and maintainability.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/PRICE.sol#L59

    uint8 public constant decimals = 18;

[N-04] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of the magic numbers. Please consider replacing the magic numbers used in the following code with constants.

modules\PRICE.sol
  90: if (exponent > 38) revert Price_InvalidParams();   

modules\RANGE.sol
  245: wallSpread_ > 10000 || 
  246: wallSpread_ < 100 || 
  247: cushionSpread_ > 10000 || 
  248: cushionSpread_ < 100 || 

policies\Governance.sol
  164: if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
  217: (totalEndorsementsForProposal[proposalId_] * 100) <

policies\Operator.sol
  103: if (configParams[1] > uint256(7 days) || configParams[1] < uint256(1 days))
  106: if (configParams[2] < uint32(10_000)) revert Operator_InvalidParams();
  108: if (configParams[3] < uint32(1 hours) || configParams[3] > configParams[1])
  111: if (configParams[4] > 10000 || configParams[4] < 100) revert Operator_InvalidParams();
  114: configParams[5] < 1 hours ||
  378: 36 + scaleAdjustment + int8(reserveDecimals) - int8(ohmDecimals) - priceDecimals   
  433: 36 + scaleAdjustment + int8(ohmDecimals) - int8(reserveDecimals) - priceDecimals   
  533: if (duration_ > uint256(7 days) || duration_ < uint256(1 days))
  535: if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams();
  536: if (depositInterval_ < uint32(1 hours) || depositInterval_ > duration_)
  550: if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams();
  565: if (wait_ < 1 hours || threshold_ > observe_ || observe_ == 0)

[N-05] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

Kernel.sol
  235: function executeAction(Actions action_, address target_) external onlyExec
  351: function _migrateKernel(Kernel newKernel_) internal   
  439: function grantRole(Role role_, address addr_) public onlyAdmin {  
  451: function revokeRole(Role role_, address addr_) public onlyAdmin {

modules\TRSRY.sol
  64: function setApprovalFor(  
  75: function withdrawReserves(  
  92: function getLoan(ERC20 token_, uint256 amount_) external permissioned {  
  105: function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant {  
  122: function setDebt(  

modules\VOTES.sol
  45: function transfer(address to_, uint256 amount_) public pure override returns (bool) {  
  51: function transferFrom(  

[N-06] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

Kernel.sol
  266: function _installModule(Module newModule_) internal {  
  279: function _upgradeModule(Module newModule_) internal {  
  295: function _activatePolicy(Policy policy_) internal {  
  325: function _deactivatePolicy(Policy policy_) internal {  
  378: function _reconfigurePolicies(Keycode keycode_) internal { 
  391: function _setPolicyPermissions( 
  409: function _pruneFromDependents(Policy policy_) internal { 

modules\MINTR.sol
  33: function mintOhm(address to_, uint256 amount_) public permissioned {   
  37: function burnOhm(address from_, uint256 amount_) public permissioned {   

modules\TRSRY.sol
  47: function KEYCODE() public pure override returns (Keycode) {  
  51: function VERSION() external pure override returns (uint8 major, uint8 minor) {  
  59: function getReserveBalance(ERC20 token_) external view returns (uint256) {  
  137: function _checkApproval(  

modules\VOTES.sol
  35: function mintTo(address wallet_, uint256 amount_) external permissioned {  
  39: function burnFrom(address wallet_, uint256 amount_) external permissioned {  

utils\KernelUtils.sol
  11: function toKeycode(bytes5 keycode_) pure returns (Keycode) {   
  16: function fromKeycode(Keycode keycode_) pure returns (bytes5) {  
  21: function toRole(bytes32 role_) pure returns (Role) {  
  26: function fromRole(Role role_) pure returns (bytes32) {  
  31: function ensureContract(address target_) view {  
  40: function ensureValidKeycode(Keycode keycode_) pure {  
  55: function ensureValidRole(Role role_) pure {  

[G-01] onlyKernel MODIFIER IS NOT NEEDED FOR VIEW FUNCTIONS

Calling the following requestPermissions view functions, which are called by non-view functions, such as _activatePolicy and _deactivatePolicy, will use gas. Because these requestPermissions functions have the onlyKernel, more run-time gas is used when calling these. Since view functions do not modify states, it is safe for these view functions to not use onlyKernel. Moreover, other similar view functions, such as Operator.requestPermissions do not use onlyKernel already. Please consider removing onlyKernel from the following view functions.

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/BondCallback.sol#L60-L76

    function requestPermissions()
        external
        view
        override
        onlyKernel
        returns (Permissions[] memory requests)
    {
        Keycode TRSRY_KEYCODE = TRSRY.KEYCODE();
        Keycode MINTR_KEYCODE = MINTR.KEYCODE();

        requests = new Permissions[](4);
        requests[0] = Permissions(TRSRY_KEYCODE, TRSRY.setApprovalFor.selector);
        requests[1] = Permissions(TRSRY_KEYCODE, TRSRY.withdrawReserves.selector);
        requests[2] = Permissions(MINTR_KEYCODE, MINTR.mintOhm.selector);
        requests[3] = Permissions(MINTR_KEYCODE, MINTR.burnOhm.selector);
    }

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L70-L80

    function requestPermissions()
        external
        view
        override
        onlyKernel
        returns (Permissions[] memory requests)
    {
        requests = new Permissions[](2);
        requests[0] = Permissions(INSTR.KEYCODE(), INSTR.store.selector);
        requests[1] = Permissions(VOTES.KEYCODE(), VOTES.transferFrom.selector);
    }

[G-02] REVERT CHECK CAN BE PLACED AT START OF FUNCTION BODY IF POSSIBLE

When a revert check is allowed to be placed at the start of the function body, the subsequent operations that cost more gas are prevented from running if it does revert.

if (length == 0) revert INSTR_InstructionsCannotBeEmpty() can be placed after uint256 length = instructions_.length; in the following code. https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/INSTR.sol#L42-L79

    function store(Instruction[] calldata instructions_) external permissioned returns (uint256) {
        uint256 length = instructions_.length;
        uint256 instructionsId = ++totalInstructions;

        Instruction[] storage instructions = storedInstructions[instructionsId];

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

        ...
    }

if (activeProposal.proposalId == 0) { revert NoActiveProposalDetected(); } and if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) { revert UserAlreadyVoted(); } can be placed above uint256 userVotes = VOTES.balanceOf(msg.sender); in the following code. https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240-L262

    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();
        }

        ...
    }

[G-03] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

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

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

[G-04] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, instructions.length in the following code can be cached outside of the loop like uint256 instructionsLength = instructions.length, and i < instructionsLength can be used for each iteration.

policies\Governance.sol
  278: for (uint256 step; step < instructions.length; ) {

[G-05] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

policies\Operator.sol
  488: decimals++;
  670: _status.low.count++;
  686: _status.high.count++;
  
utils\KernelUtils.sol
  49: i++;
  64: i++;

[G-06] X = X + Y CAN BE USED INSTEAD OF X += Y

x = x + y costs less gas than x += y. For example, balanceOf[to_] += amount_ can be changed to balanceOf[to_] = balanceOf[to_] + amount_ in the following code.

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

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

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

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

policies\Governance.sol
  198: totalEndorsementsForProposal[proposalId_] += userVotes;
  252: yesVotesForProposal[activeProposal.proposalId] += userVotes;
  254: noVotesForProposal[activeProposal.proposalId] += userVotes;

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