Olympus DAO contest - Sm4rty's results

Lines of code

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

Vulnerability details

Impact

The calls to the latestRoundData() function do not validate the output of the Chainlink oracle query. As a result, it is possible to use stale results when returning the price. latestRoundData() is able to ensure the round is complete and has returned a valid/expected price by validating additional round data. This is documented in chainlink docs. However, there are some missing validations that can be improved upon the existing implementation.

Proof of Concept

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

src/modules/PRICE.sol:161:            (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
src/modules/PRICE.sol:170:            (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();

Recommended Mitigation Steps

Consider performing proper validation of Chainlink's latestRoundData() function. This can be updated or adapted to match the following code snippet:

 (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
require(ohmEthPriceInt > 0, "Chainlink Malfunction");

1. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

VOTES.sol:L36

src/modules/VOTES.sol:36:        _mint(wallet_, amount_);

Recommendations:

Use _safeMint() instead of _mint().

---

2. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances:

Operator.sol:L657 TreasuryCustodian.sol:L51 TreasuryCustodian.sol:L52 TreasuryCustodian.sol:L56

src/policies/Operator.sol:657:        /// 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?
src/policies/TreasuryCustodian.sol:51:    // TODO Currently allows anyone to revoke any approval EXCEPT activated policies.
src/policies/TreasuryCustodian.sol:52:    // TODO must reorg policy storage to be able to check for deactivated policies.
src/policies/TreasuryCustodian.sol:56:        // TODO Make sure `policy_` is an actual policy and not a random address.

References:

https://code4rena.com/reports/2022-05-sturdy/#l-09-open-todos

---

3. safeApprove() is Deprecated.

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219. The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

Instances:

BondCallback.sol:L57 Operator.sol:L167

src/policies/BondCallback.sol:57:        ohm.safeApprove(address(MINTR), type(uint256).max);
src/policies/Operator.sol:167:        ohm.safeApprove(address(MINTR), type(uint256).max);

References:

https://code4rena.com/reports/2022-02-hubble/#l-07-deprecated-safeapprove-function

---

4. Variable names that consist of all capital letters should be reserved for const/immutable variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

Instances:

https://github.com/code-423n4/2022-08-olympus/blob/main/ https://github.com/code-423n4/2022-08-olympus/blob/main/ https://github.com/code-423n4/2022-08-olympus/blob/main/

src/policies/Heart.sol:45:    OlympusPrice internal PRICE;
src/policies/PriceConfig.sol:11:    OlympusPrice internal PRICE;
src/policies/Operator.sol:69:    OlympusPrice internal PRICE;

References:

https://code4rena.com/reports/2022-05-sturdy#n-08-variable-names-that-consist-of-all-capital-letters-should-be-reserved-for-constimmutable-variables

---

5.Multiple initialization due to initialize function not having initializer modifier.

Description

The attacker can initialize the contract, take malicious actions, and allow it to be re-initialized by the project without any error being noticed.

Instances

PriceConfig.sol:L45 Operator.sol:L598 IOperator.sol:L125

// actual codes used
policies/PriceConfig.sol:45:    function initialize(uint256[] memory startObservations_, uint48 lastObservationTime_)
policies/Operator.sol:598:    function initialize() external onlyRole("operator_admin") {
policies/interfaces/IOperator.sol:125:    function initialize() external;

6. The non Reentrant modifier should occur before all other modifiers

Instances

Operator.sol:L276

src/policies/Operator.sol:276:    ) external override onlyWhileActive nonReentrant returns (uint256 amountOut) {

Recommended Mitigation Steps

using the nonReentrant modifier before onlyWhileActive modifier

---

1. Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Instances

PRICE.sol:L143 PRICE.sol:L146 PRICE.sol:L165 PRICE.sol:L171 PRICE.sol:L215 RANGE.sol:L85 RANGE.sol:L92 RANGE.sol:L136 RANGE.sol:L138 RANGE.sol:L148 RANGE.sol:L150 RANGE.sol:L191 RANGE.sol:L200 RANGE.sol:L207 RANGE.sol:L231 RANGE.sol:L233 Heart.sol:L63 Heart.sol:L94 Heart.sol:L108 Heart.sol:L131 Operator.sol:L128 Operator.sol:L210 Operator.sol:L216 Operator.sol:L404 Operator.sol:L456 Operator.sol:L708 Operator.sol:L720 Governance.sol:L171 Governance.sol:L212 Governance.sol:L227 Governance.sol:L231 Governance.sol:L235 Governance.sol:L272

src/modules/PRICE.sol:143:        lastObservationTime = uint48(block.timestamp);
src/modules/PRICE.sol:146:        emit NewObservation(block.timestamp, currentPrice, _movingAverage);
src/modules/PRICE.sol:165:            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
src/modules/PRICE.sol:171:            if (updatedAt < block.timestamp - uint256(observationFrequency))
src/modules/PRICE.sol:215:        if (startObservations_.length != numObs || lastObservationTime_ > uint48(block.timestamp))
src/modules/RANGE.sol:85:                lastActive: uint48(block.timestamp),
src/modules/RANGE.sol:92:                lastActive: uint48(block.timestamp),
src/modules/RANGE.sol:136:                _range.high.lastActive = uint48(block.timestamp);
src/modules/RANGE.sol:138:                emit WallDown(true, block.timestamp, capacity_);
src/modules/RANGE.sol:148:                _range.low.lastActive = uint48(block.timestamp);
src/modules/RANGE.sol:150:                emit WallDown(false, block.timestamp, capacity_);
src/modules/RANGE.sol:191:                lastActive: uint48(block.timestamp),
src/modules/RANGE.sol:200:                lastActive: uint48(block.timestamp),
src/modules/RANGE.sol:207:        emit WallUp(high_, block.timestamp, capacity_);
src/modules/RANGE.sol:231:            emit CushionDown(high_, block.timestamp);
src/modules/RANGE.sol:233:            emit CushionUp(high_, block.timestamp, marketCapacity_);
src/policies/Heart.sol:63:        lastBeat = block.timestamp;
src/policies/Heart.sol:94:        if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();
src/policies/Heart.sol:108:        emit Beat(block.timestamp);
src/policies/Heart.sol:131:        lastBeat = block.timestamp - frequency();
src/policies/Operator.sol:128:            lastRegen: uint48(block.timestamp),
src/policies/Operator.sol:210:            uint48(block.timestamp) >= RANGE.lastActive(true) + uint48(config_.regenWait) &&
src/policies/Operator.sol:216:            uint48(block.timestamp) >= RANGE.lastActive(false) + uint48(config_.regenWait) &&
src/policies/Operator.sol:404:                conclusion: uint48(block.timestamp + config_.cushionDuration),
src/policies/Operator.sol:456:                conclusion: uint48(block.timestamp + config_.cushionDuration),
src/policies/Operator.sol:708:            _status.high.lastRegen = uint48(block.timestamp);
src/policies/Operator.sol:720:            _status.low.lastRegen = uint48(block.timestamp);
src/policies/Governance.sol:171:            block.timestamp,
src/policies/Governance.sol:212:        if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
src/policies/Governance.sol:227:        if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
src/policies/Governance.sol:231:        activeProposal = ActivatedProposal(proposalId_, block.timestamp);
src/policies/Governance.sol:235:        emit ProposalActivated(proposalId_, block.timestamp);
src/policies/Governance.sol:272:        if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

---

1. Pre-increment costs less gas as compared to Post-increment :

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)

i++ increments i and returns initial value of i. Which means

uint i =  1;
i++; // ==1 but i ==2

But ++i returns the actual incremented value:

uint i = 1;
++i; // ==2 and i ==2 , no need for temporary variable here

In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.

Instances:

KernelUtils.sol:L49 KernelUtils.sol:L64 Operator.sol:L488 Operator.sol:L670 Operator.sol:L686 Operator.sol:L675 Operator.sol:L691

src/utils/KernelUtils.sol:49:            i++;
src/utils/KernelUtils.sol:64:            i++;
src/policies/Operator.sol:488:            decimals++;
src/policies/Operator.sol:670:                _status.low.count++;
src/policies/Operator.sol:686:                _status.high.count++;
src/policies/Operator.sol:675:                _status.low.count--;
src/policies/Operator.sol:691:                _status.high.count--;

Recommendations:

Change post-increment to pre-increment.

---

2. An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Instances:

Governance.sol:L278

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

Remediation:

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead.

---

3. Boolean Comparision

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here

Instances:

Governance.sol:L223 Governance.sol:L306

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

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-04-fundingonlywhenpricenotflagged-boolean-comparison-147

---

4. Strict inequalities (>) are more expensive than non-strict ones (>=)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas. I suggest using >= instead of > to avoid some opcodes here:

Instances:

RANGE.sol:L245 RANGE.sol:L247 RANGE.sol:L249 Operator.sol:L115 Operator.sol:L254 Operator.sol:L262

src/modules/RANGE.sol:245:            wallSpread_ > 10000 ||
src/modules/RANGE.sol:247:            cushionSpread_ > 10000 ||
src/modules/RANGE.sol:249:            cushionSpread_ > wallSpread_
src/policies/Operator.sol:115:            configParams[6] > configParams[7] ||
src/policies/Operator.sol:254:                    currentPrice < range.cushion.high.price || currentPrice > range.wall.high.price
src/policies/Operator.sol:262:                    currentPrice > range.cushion.high.price && currentPrice < range.wall.high.price

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-31--is-cheaper-than

---

5. x += y costs more gas than x = x + y for state variables

Instances:

PRICE.sol:L136 PRICE.sol:L222 TRSRY.sol:L96 TRSRY.sol:L97 TRSRY.sol:L131 VOTES.sol:L58 Heart.sol:L103 BondCallback.sol:L143 BondCallback.sol:L144 Governance.sol:L198 Governance.sol:L252 Governance.sol:L254 PRICE.sol:L138 TRSRY.sol:L115 TRSRY.sol:L116 TRSRY.sol:L132 VOTES.sol:L56 Governance.sol:L194

src/modules/PRICE.sol:136:            _movingAverage += (currentPrice - earliestPrice) / numObs;
src/modules/PRICE.sol:222:            total += startObservations_[i];
src/modules/TRSRY.sol:96:        reserveDebt[token_][msg.sender] += amount_;
src/modules/TRSRY.sol:97:        totalDebt[token_] += amount_;
src/modules/TRSRY.sol:131:        if (oldDebt < amount_) totalDebt[token_] += amount_ - oldDebt;
src/modules/VOTES.sol:58:            balanceOf[to_] += amount_;
src/policies/Heart.sol:103:        lastBeat += frequency();
src/policies/BondCallback.sol:143:        _amountsPerMarket[id_][0] += inputAmount_;
src/policies/BondCallback.sol:144:        _amountsPerMarket[id_][1] += outputAmount_;
src/policies/Governance.sol:198:        totalEndorsementsForProposal[proposalId_] += userVotes;
src/policies/Governance.sol:252:            yesVotesForProposal[activeProposal.proposalId] += userVotes;
src/policies/Governance.sol:254:            noVotesForProposal[activeProposal.proposalId] += userVotes;
src/modules/PRICE.sol:138:            _movingAverage -= (earliestPrice - currentPrice) / numObs;
src/modules/TRSRY.sol:115:        reserveDebt[token_][msg.sender] -= received;
src/modules/TRSRY.sol:116:        totalDebt[token_] -= received;
src/modules/TRSRY.sol:132:        else totalDebt[token_] -= oldDebt - amount_;
src/modules/VOTES.sol:56:        balanceOf[from_] -= amount_;
src/policies/Governance.sol:194:        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

References:

https://github.com/code-423n4/2022-05-backd-findings/issues/108

---

6. Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.

Refer Here Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Instances:

PRICE.sol:L62 RANGE.sol:L44 Kernel.sol:L113 Kernel.sol:L181 Kernel.sol:L194 Kernel.sol:L197 Heart.sol:L33 BondCallback.sol:L24 Operator.sol:L63 Operator.sol:L66 Operator.sol:L735 Governance.sol:L105 Governance.sol:L117

src/modules/PRICE.sol:62:    bool public initialized;
src/modules/RANGE.sol:44:        bool active; // Whether or not the side is active (i.e. the Operator is performing market operations on this side, true = active, false = inactive)
src/Kernel.sol:113:    bool public isActive;
src/Kernel.sol:181:    mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions;
src/Kernel.sol:194:    mapping(address => mapping(Role => bool)) public hasRole;
src/Kernel.sol:197:    mapping(Role => bool) public isRole;
src/policies/Heart.sol:33:    bool public active;
src/policies/BondCallback.sol:24:    mapping(address => mapping(uint256 => bool)) public approvedMarkets;
src/policies/Operator.sol:63:    bool public initialized;
src/policies/Operator.sol:66:    bool public active;
src/policies/Operator.sol:735:        bool sideActive = RANGE.active(high_);
src/policies/Governance.sol:105:    mapping(uint256 => bool) public proposalHasBeenActivated;
src/policies/Governance.sol:117:    mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;

References:

https://code4rena.com/reports/2022-06-notional-coop#8-using-bools-for-storage-incurs-overhead

---

7. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

Instances:

PRICE.sol:L59 PRICE.sol:L84 PRICE.sol:L87 Operator.sol:L83 Operator.sol:L86 Operator.sol:L418

src/modules/PRICE.sol:59:    uint8 public constant decimals = 18;
src/modules/PRICE.sol:84:        uint8 ohmEthDecimals = _ohmEthPriceFeed.decimals();
src/modules/PRICE.sol:87:        uint8 reserveEthDecimals = _reserveEthPriceFeed.decimals();
src/policies/Operator.sol:83:    uint8 public immutable ohmDecimals;
src/policies/Operator.sol:86:    uint8 public immutable reserveDecimals;
src/policies/Operator.sol:418:            uint8 oracleDecimals = PRICE.decimals();

References:

https://code4rena.com/reports/2022-04-phuture#g-14-usage-of-uintsints-smaller-than-32-bytes-256-bits-incurs-overhead

---

8. Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Instances:

IBondAggregator.sol:L2 IOperator.sol:L2 IHeart.sol:L2

src/interfaces/IBondAggregator.sol:2:pragma solidity >=0.8.0;
src/policies/interfaces/IOperator.sol:2:pragma solidity >=0.8.0;
src/policies/interfaces/IHeart.sol:2:pragma solidity >=0.8.0;

References:

https://code4rena.com/reports/2022-06-notional-coop/#10-use-a-more-recent-version-of-solidity

---

9. Use Shift Right/Left instead of Division/Multiplication 2X

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity’s division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

Instances

Operator.sol:L372 Operator.sol:L427 Operator.sol:L419 Operator.sol:L420 Operator.sol:L786

src/policies/Operator.sol:372:            int8 scaleAdjustment = int8(ohmDecimals) - int8(reserveDecimals) + (priceDecimals / 2);
src/policies/Operator.sol:427:            int8 scaleAdjustment = int8(reserveDecimals) - int8(ohmDecimals) + (priceDecimals / 2);
src/policies/Operator.sol:419:            uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
src/policies/Operator.sol:420:            uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;
src/policies/Operator.sol:786:                ) * (FACTOR_SCALE + RANGE.spread(true) * 2)) /

---

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-32-shift-right-instead-of-dividing-by-2

10. Inline a modifier that’s only used once

As onlyGovernor() is only used once in this contract (in function executeAction()), it should get inlined to save gas:

Instances

Kernel.sol:L223

src/Kernel.sol:223:    modifier onlyExecutor() {

Kernel.sol:L235

src/Kernel.sol:235:    function executeAction(Actions action_, address target_) external onlyExecutor {

---

11. Bytes constants are more efficient than string constants

From the Solidity doc: If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper. Why do Solidity examples use bytes32 type instead of string? bytes32 uses less gas because it fits in a single word of the EVM, and string is a dynamically sized-type which has current limitations in Solidity (such as can’t be returned from a function to a contract). If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Instances of string constant that can be replaced by bytes(1..32) constant : Governance.sol:L41 Governance.sol:L162

policies/Governance.sol:41:    string proposalURI;
policies/Governance.sol:162:        string memory proposalURI_