Olympus DAO contest - Rohan16's results

1. safeApprove() is deprecated

Description

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().Whenever possible, use {safeIncreaseAllowance} and {safeDecreaseAllowance} instead.

Instances

//Links to github Files: Operator.sol:L167 BondCallback.sol:L57

Actual Codes Used

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

---

2. Avoid using Floating Pragma:

Description

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. The pragma declared across the solution is ^0.8.0 As the compiler introduces a several interesting upgrades in newer versions of Solidity consider locking at this version or a more recent one.

Instances

//Links to github Files: IBondCallback.sol:L2 IOperator.sol:L2 IHeart.sol:L2

Actual Codes Used

src/interfaces/IBondCallback.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;

---

3.Use of Block.timestamp

Impact - Non-Critical

Description

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

// Link to Github files: 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

Actual Codes Used

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.

---

4. The nonReentrant modifier should occur before all other modifiers

Description

The nonReentrant modifier should occur before all other modifiers This is a best-practice to protect against re-entrancy in other modifiers

Instances

// Links To Github Files: Operator.sol:L276

Actual Codes Used

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

Recommended Mitigation Steps

using the nonReentrant modifier before onlyWhileActive modifier

---

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

Description

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver.

Instances

// Links To Github Files: VOTES.sol:L36

Actual Codes Used

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

Recommendations:

Use _safeMint() instead of _mint().

---

6.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

// Links to githubfile PriceConfig.sol:L45 Operator.sol:L598 IOperator.sol:L125

Actual Codes Used

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

---

7. Open TODOs

Description

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

Instances

// Link to Github files: Operator.sol:L657 TreasuryCustodian.sol:L51 TreasuryCustodian.sol:L52 TreasuryCustodian.sol:L56

Actual Codes Used

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.

---

1. Inline a modifier that’s only used once

Description

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

Instances

//Links to github files: Kernel.sol:L223

Actual codes used

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

Instances where modifiers are used only once

//Links to github files: Kernel.sol:L235

Actual codes used

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

---

2.++I COSTS LESS GAS COMPARED TO I++ OR I += 1

Description

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k: Increment: i += 1 is the most expensive form i++ costs 6 gas less than i += 1 ++i costs 5 gas less than i++ (11 gas less than i += 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

Instances

// Links to Github file Operator.sol:L488 Operator.sol:L670 Operator.sol:L686 KernelUtils.sol:L49 KernelUtils.sol:L64

Actual codes used

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

---

3.<ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP

Description

The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

Instances

// Links to github files Governance.sol:L278

Actual codes used

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

---

4. Comparisons: Boolean comparisons

Description

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)

Instances

//Links to github files: Governance.sol:L223 Governance.sol:L306

Actual codes used

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

---

5. IT COSTS MORE GAS TO INITIALIZE VARIABLES WITH THEIR DEFAULT VALUE THAN LETTING THE DEFAULT VALUE BE APPLIED

Description

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

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances

// Links to gihthub file

Kernel.sol:L397 KernelUtils.sol:L43 KernelUtils.sol:L58

Actual codes used

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

---

6. 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

---

7. Use a more recent version of solidity

Description

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get 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:

//Links to github files: IBondCallback.sol:L2 IOperator.sol:L2 IHeart.sol:L2

Actual codes used

src/interfaces/IBondCallback.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;

---

8. Bytes constants are more efficient than string constants

Descriptions

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 :

Instances:

//Links to github files: Governance.sol:L41 Governance.sol:L162

Actual codes used

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