Olympus DAO contest - delfin454000's results

Typos

PRICE.sol: L126

        // Cache numbe of observations to save gas.

Change numbe to number

---

The same typo (deactive) occurs in both lines referenced below:

Operator.sol: L295

Operator.sol: L326

            /// If wall is down after swap, deactive the cushion as well

Change deactive to deactivate in both cases

---

---

Long single line comments

In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable and a scroll bar is provided, very long comments can interfere with readability. Some of the long comments in Olympus do wrap (e.g., PRICE.sol: L237-239) — the treatment of very long comments is inconsistent and should be made consistent. Below is an example of a line that should wrap:

PRICE.sol: L31

    /// @dev    Price feeds. Chainlink typically provides price feeds for an asset in ETH. Therefore, we use two price feeds against ETH, one for OHM and one for the Reserve asset, to calculate the relative price of OHM in the Reserve asset.

Suggestion:

    /// @dev    Price feeds. Chainlink typically provides price feeds for an asset in ETH. 
    ///         Therefore, we use two price feeds against ETH — one for OHM and one for the
    ///         Reserve asset — to calculate the relative price of OHM in the Reserve asset.

Similarly for the following 10 examples:

RANGE.sol: L40

RANGE.sol: L44

RANGE.sol: L46

RANGE.sol: L47

RANGE.sol: L61

PRICE.sol: L120

Operator.sol: L657

IOperator.sol: L15

IOperator.sol: L90

IOperator.sol: L91

---

---

Comments concerning unfinished work or open items should be addressed

Comments that contain TODOs or other language referring to open items should be addressed and modified or removed. Below are two instances:

---

TreasuryCustodian.sol: L50-56

    // Anyone can call to revoke a deactivated policy's approvals.
    // 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.

---

Operator.sol: L656-659

        /// Get latest price
        /// 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?
        /// Current price is guaranteed to be up to date, but may be a bad value if not checked?
        uint256 currentPrice = PRICE.getLastPrice();

---

---

Update sensitive terms in both comments and code

Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice. Example:

Operator.sol: L411-412

            /// Whitelist the bond market on the callback
            callback.whitelist(address(auctioneer.getTeller()), market);

Suggestion: Change whitelist to 'allowlist`

---

Similarly for the following six instances (change whitelisted to allowlisted where it occurs):

Operator.sol: L463-464

BondCallback.sol: L83-87

BondCallback.sol: L105

IBondCallback.sol: L8

IBondCallback.sol: L26

IBondCallback.sol: L30

---

---

Use consistent initialization of counters in for loops

Some for loop counters in the contracts are initiated to zero (uint256 i = 0;) whereas others are not (uint256 i;). It is not necessary to initialize for loop counters to zero since this is their default value. For consistency, it makes sense to omit such initializations in the for loops below:

---

Kernel.sol: L397-406

KernelUtils.sol: L43-51

KernelUtils.sol: L58-66

---

---

Array length should not be looked up in every iteration of a for loop

Since calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop.

---

Governance.sol: L278-283

        for (uint256 step; step < instructions.length; ) {
            kernel.executeAction(instructions[step].action, instructions[step].target);
            unchecked {
                ++step;
            }
        }

Suggestion:

        uint256 instructLength = instructions.length;
        for (uint256 step; step < instructLength; ) {
            kernel.executeAction(instructions[step].action, instructions[step].target);
            unchecked {
                ++step;
            }
        }

---

---

Use ++i instead of i++ to increase count in a for loop

Since use of i++ (or equivalent counter) costs more gas, it is better to use ++i

---

KernelUtils.sol: L43-51

    for (uint256 i = 0; i < 5; ) {
        bytes1 char = unwrapped[i];

        if (char < 0x41 || char > 0x5A) revert InvalidKeycode(keycode_); // A-Z only

        unchecked {
            i++;
        }
    }

Suggestion:

    for (uint256 i = 0; i < 5; ) {
        bytes1 char = unwrapped[i];

        if (char < 0x41 || char > 0x5A) revert InvalidKeycode(keycode_); // A-Z only

        unchecked {
            ++i;
        }
    }

---

Similarly for the following for loop:

KernelUtils.sol: L58-66

---

---