Olympus DAO contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the vote function of the OlympusGovernance contract, each user can only vote once for an activeProposal, which prevents the user from voting for the activeProposal even if new VOTES are obtained after voting. Consider the following scenario:

1. User A calls the vote function to vote for an activeProposal
2. voter_admin calls the issueVotesTo function of the VoterRegistration contract to assign new VOTES to user A, or user A calls the reclaimVotes function to reclaim VOTES
3. Since user A's userVotesForProposal > 0, user A cannot use the newly obtained VOTES to vote for the activeProposal

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

In the vote function, when the user's balance is greater than userVotesForProposal, the user is allowed to vote using the difference

Lines of code

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

Vulnerability details

Impact

In the VoterRegistration contract, voter_admin can call the revokeVotesFrom function to revoke the user's votes.

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

But there is a way for users to prevent their votes from being revoked by voter_admin. In the OlympusGovernance contract, the user can call the vote function to vote for the activeProposal, and then call the reclaimVotes function to reclaim his votes. When the vote function is called, VOTES are sent to the OlympusGovernance contract and recorded using the userVotesForProposal variable. When the reclaimVotes function is called, the VOTES recorded in the userVotesForProposal variable are sent back to the user. This means that the user can store his VOTES tokens in userVotesForProposal. The revokeVotesFrom function cannot revoke the VOTES tokens recorded in userVotesForProposal and the reclaimVotes function can only be called by the user himself. If the user calls the reclaimVotes function and vote function in one transaction, then his VOTES token balance will always be 0 (thus avoiding revocation of votes by voter_admin) and he will be able to vote.

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L240-L262 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L295-L313 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/VoterRegistration.sol#L53-L56

Tools Used

None

Recommended Mitigation Steps

Consider allowing to call the reclaimVotes function to reclaim any user's vote, thus avoiding the user storing his VOTOS tokens in userVotesForProposal

    function reclaimVotes(uint256 proposalId_, address user_) external {
        uint256 userVotes = userVotesForProposal[proposalId_][user_];

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

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

        if (tokenClaimsForProposal[proposalId_][user_] == true) {
            revert VotingTokensAlreadyReclaimed();
        }

        tokenClaimsForProposal[proposalId_][user_] = true;

        VOTES.transferFrom(address(this), user_, userVotes);
    }

Lines of code

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

Vulnerability details

Impact

In the beat function of the OlympusHeart contract, the user will receive reward tokens by spending gas to update the price.

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

But if there are not enough reward tokens in the contract, _issueReward will fail and the price cannot be updated.

    function _issueReward(address to_) internal {
        rewardToken.safeTransfer(to_, reward);
        emit RewardIssued(to_, reward);
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider limiting the number of reward tokens sent to the user to no more than the contract balance in the _issueReward function. And the contract can store the number of reward tokens for the user to claim later

Lines of code

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

Vulnerability details

Impact

On Price.sol, we are using latestRoundData, but there is no check if the return value indicates stale data.

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

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

-           (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
+          (uint80 roundID, int256 ohmEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _ohmEthPriceFeed.latestRoundData();
+          require(answeredInRound >= roundID, "Stale price");
+          require(ohmEthPriceInt > 0,"Chainlink answer reporting 0");
            // 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();
+          (uint80 roundID, int256 reserveEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _reserveEthPriceFeed.latestRoundData();
+          require(answeredInRound >= roundID, "Stale price");
+          require(ohmEthPriceInt > 0,"Chainlink answer reporting 0");

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

[Low-01] ensureValidKeycode can be bypassed

Impact

In the executeAction function of the Kernel contract, when the action is InstallModule or UpgradeModule, the ensureValidKeycode function is used to verify that Module(target_).KEYCODE() consists of uppercase letters. But after passing ensureValidKeycode verification, newModule_.KEYCODE() will be used again as keycode in _installModule and _upgradeModule functions. Users can bypass ensureValidKeycode() verification by controlling the return value of KEYCODE()

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/Kernel.sol#L235-L243 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/Kernel.sol#L265-L293

Recommended Mitigation Steps

Consider getting the keycode only once with KEYCODE() and use the keycode later instead of KEYCODE()

[Low-02] setRewardTokenAndAmount() will front run beat()

Impact

In the OlympusHeart contract, heart_admin can call setRewardTokenAndAmount to set the reward tokens and reward amount. When the setRewardTokenAndAmount function is executed in the same block as the beat function, it may front run the user's beat, causing the user to receive less reward.

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Heart.sol#L92-L114 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Heart.sol#L140-L147

Recommended Mitigation Steps

[Low-03] The constructor of the Operator does not restrict the cushionFactor

Impact

There is no restriction on configParams[0] in the constructor of Operator. configParams[0] will be assigned to cushionFactor directly afterwards. In the setCushionFactor function, the cushionFactor_ is limited to be greater than 100 and less than 10000.

    function setCushionFactor(uint32 cushionFactor_) external onlyRole("operator_policy") {
        /// Confirm factor is within allowed values
        if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();

        /// Set factor
        _config.cushionFactor = cushionFactor_;

        emit CushionFactorChanged(cushionFactor_);
    }

If an incorrect cushionFactor is set in the constructor of the Operator, this will work because there is no restriction. Later the _activate function will make the created market capacity too large or too small, and may even overflow to make the contract not work

            uint256 marketCapacity = range.high.capacity.mulDiv(
                config_.cushionFactor,
                FACTOR_SCALE
            );

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L92-L134 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L516-L524 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L388-L391

Recommended Mitigation Steps

In the constructor of the Operator, limit cushionFactor_ to be greater than 100 and less than 10000.

[Low-04] OlympusPrice: observations have no minimum length limit

Impact

The observations array of the OlympusPrice contract is used to calculate the moving average price, which is usually used to prevent price manipulation. When the length of observations is too small (currently the minimum length is 1), price manipulation cannot be prevented.

Proof of Concept

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L97-L100 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L242-L249 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L268-L281

Recommended Mitigation Steps

Consider adding a minimum length limit to observations

[Low-05] If the beat function of the OlympusHeart contract is not called for a long time, the user can control the moving average price by continuously calling beat

Impact

When the reward is unattractive (gas consumption is greater than reward tokens) and the price is stable, people have no incentive to call OlympusHeart's beat function to update the price. After a period of time, if the attacker launches a price manipulation attack, the attacker can control the moving average price by continuously calling the beat function. This is because when the beat function is called, lastBeat will not be updated to the current time, but will add a value.

Proof of Concept

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

Recommended Mitigation Steps

Consider updating lastBeat to the current time in the beat function

[G-01] OlympusPrice.constructor(): SHOULD USE MEMORY INSTEAD OF STORAGE VARIABLE

See @audit tag

        _ohmEthPriceFeed = ohmEthPriceFeed_;
        uint8 ohmEthDecimals = _ohmEthPriceFeed.decimals(); // @audit gas: should use ohmEthPriceFeed_.decimals()

        _reserveEthPriceFeed = reserveEthPriceFeed_;
        uint8 reserveEthDecimals = _reserveEthPriceFeed.decimals(); // @audit gas: should use reserveEthPriceFeed_.decimals()

[G-02] OlympusPrice.updateMovingAverage(): L136 AND L138 SHOULD BE UNCHECKED DUE TO L135

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.7/control-structures.html#checked-or-unchecked-arithmetic

135:        if (currentPrice > earliestPrice) {
136:            _movingAverage += (currentPrice - earliestPrice) / numObs;
137:        } else {
138:            _movingAverage -= (earliestPrice - currentPrice) / numObs;
139:        }

[G-03] OlympusGovernance.executeProposal : kernel SHOULD GET CACHED

See @audit tag

        for (uint256 step; step < instructions.length; ) {
            kernel.executeAction(instructions[step].action, instructions[step].target); // @audit gas: should cache "kernel" (SLOAD IN LOOP)
            unchecked {
                ++step;
            }
        }

[G-04] OlympusGovernance.vote : > 0 IS LESS EFFICIENT THAN != 0 FOR UNSIGNED INTEGERS

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

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

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

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) { // @audit gas: should use userVotesForProposal[activeProposal.proposalId][msg.sender] != 0
            revert UserAlreadyVoted();
        }

[G-05] TreasuryCustodian.revokePolicyApprovals : TRSRY SHOULD GET CACHED

See @audit tag

        for (uint256 j; j < len; ) {
            TRSRY.setApprovalFor(policy_, tokens_[j], 0); // @audit gas: should cache "TRSRY" (SLOAD IN LOOP)
            unchecked {
                ++j;
            }
        }