Olympus DAO contest - GalloDaSballo's results

Lines of code

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

Vulnerability details

vote allows a user to vote only once.

Because of this, if a person where to cast a vote, and then increase their VOTE balance, their vote would only reflect the old balance, without any way to vote again for the activeProposal.

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

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

POC

• I have 10 new VOTES, I forgot to claim some from the old proposal.
• I vote
• I realize my vote is low
• I claim back the old votes and now I cannot vote again

Remediation Steps

Because userVotesForProposal[activeProposal.proposalId][msg.sender] already tracks the vote amount, just subtract the old votes and add a new one, allowing people to re-cast the vote if their VOTE balance changes

Lines of code

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

Vulnerability details

Rewards for Heart beat are sent via _issueReward

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

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

The function doesn't check for available tokens e.g. min(reward, rewardToken.balanceOf(address(this)));

In case of calling withdrawUnspentRewards

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

    /// @inheritdoc IHeart
    function withdrawUnspentRewards(ERC20 token_) external onlyRole("heart_admin") {
        token_.safeTransfer(msg.sender, token_.balanceOf(address(this)));
    }

Because the function withdraws the entire amount, the heart will stop until a caller incentive is deposited again.

While a profitable searches will stop calling the Heart without an incentive, allowing the heart to beat when no rewards are available is preferable to having it self-DOS until a DAO aligned caller donates rewardToken or the DAO deals with the lack of tokens.

Remediation

Add a check for available tokens min(reward, rewardToken.balanceOf(address(this)));

Lines of code

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

Vulnerability details

Because VOTES can be minted by voter_admin, and there is no cap on totalSupply, the voter_admin has the privileged ability to mint as many VOTES as they want in order to get any proposal to pass or veto it.

POC - Veto

• Be voter_admin
• Mint XYZ tokens
• totalSupply is now higher so any proposal can be vetoed per this line

POC - Approve

• Be voter_admin
• Mint XYZ tokens, where XYZ allows the netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD check to pass
• Mint VOTES to self
• Vote
• Proposal has passed

Remediation Step

Add a total supply cap to VOTES

Executive Summary

The idea of Modules and Policies is brilliant!

Most of the codebase is well written and well thought out, the one exception to me was Governance which I don't believe will withstand an adversarial environment.

Minor Code smells are listed below rated via the following standard

Legend:

• L -> Low Severity -> Could cause issues however impact / probability is limited
• R -> Refactoring -> Suggested Code Change to improve readability and maintainability or to offer better User Experience
• NC -> Non-Critical / Informational -> No risk of loss, pertains to events or has no impact

L - Burning VOTES from Governance will break accounting

While burning VOTES from the Governance contract is questionable, the code has no check to prevent that.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/VOTES.sol#L38-L42

    function burnFrom(address wallet_, uint256 amount_) external permissioned {
        _burn(wallet_, amount_);
    }

Because Governance and VOTES.transferFrom relies on a "use -> refund" pattern, losing even 1 wei of token will cause reclaimVotes to revert, effectively denying a user from being able to vote again.

Voting can be denied by simply burning their VOTES hence why I set the severity to Low as this is a Ban with extra steps as the voter_admin can just burn the votes from the user

L - Allow others to repay the debt

repayLoan allows only the caller to repay their own debt, this can create situations in which insolvency or a smart contract bug prevent from making the TRSRY whole.

A straightforward solution would be to allow anyone to repay the loan on behalf of a specific address

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L103-L110

    /// @notice Lets an address with debt repay their loan.
    function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant {
        if (reserveDebt[token_][msg.sender] == 0) revert TRSRY_NoDebtOutstanding();

        // Deposit from caller first (to handle nonstandard token transfers)
        uint256 prevBalance = token_.balanceOf(address(this));
        token_.safeTransferFrom(msg.sender, address(this), amount_);

By allowing other addresses a softer approach to repaying debt can be achieved.

This avoids having to manually reset the debt.

L - _activatePolicy is non CEI conformant

The function _activatePolicy will perform an external call to policy_.configureDependencies() and then it will change storage.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L298-L315

        // Add policy to list of active policies
        activePolicies.push(policy_);
        getPolicyIndex[policy_] = activePolicies.length - 1;

        // Record module dependencies
        Keycode[] memory dependencies = policy_.configureDependencies();
        uint256 depLength = dependencies.length;

        for (uint256 i; i < depLength; ) {
            Keycode keycode = dependencies[i];

            moduleDependents[keycode].push(policy_);
            getDependentIndex[keycode][policy_] = moduleDependents[keycode].length - 1;

            unchecked {
                ++i;
            }
        }

I wasn't able to find any exploit as the function is privileged

R - get for a state changing function

getXyz is typically used for retrieving values from view functions, however in the case of TRSRY the function is used to receive a loan.

Because the codebase already uses get for view functions, I'd recommend renaming the function below to receiveLoan or just loan to keep the coding convention

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L92-L93

    function getLoan(ERC20 token_, uint256 amount_) external permissioned {

R - Can check contract existence without assembly

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/utils/KernelUtils.sol#L31-L37

function ensureContract(address target_) view {
    uint256 size;
    assembly {
        size := extcodesize(target_)
    }
    if (size == 0) revert TargetNotAContract(target_);
}

Can be changed to

target_.code.length

Lack of Address(0) Zero-Checks

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L66-L67

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

NC - Lack of event for setters

Throughout the codebase, most setters emit events, however setActiveStatus doesn't

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L127-L128

        isActive = activate_;

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

NC - Events not emitted in constructor

While setters emit events, the constructor doesn't, this may cause issues with tracking, e.g. theGraph as an event is for the initial setting is not emitted

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L217-L220

    constructor() {
        executor = msg.sender;
        admin = msg.sender;
    }

NC - Gibberish action will still emit an event

You may instead want to emit only when a valid action is executed Or add a comment to the function mentioning that

As it stands the code will emit even if the action data is not recognized

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L259-L260

        emit ActionExecuted(action_, target_);

Executive Summary

Codebase is gas conscious and basic gas saving advice is followed pretty thoroughly, below are listed a few extra optimizations, sorted by efficacy

Optimized updateMovingAverage - 200+ gas saved per function call

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L133-L145

        // Calculate new moving average
        if (currentPrice > earliestPrice) {
            _movingAverage += (currentPrice - earliestPrice) / numObs;
        } else {
            _movingAverage -= (earliestPrice - currentPrice) / numObs;
        }

        // Push new observation into storage and store timestamp taken at
        observations[nextObsIndex] = currentPrice;
        lastObservationTime = uint48(block.timestamp);
        nextObsIndex = (nextObsIndex + 1) % numObs;

Can be changed to

        // Calculate new moving average

        /// @audit Use unchecked as you already checked for overflow
        unchecked {
          if (currentPrice > earliestPrice) {
              _movingAverage += (currentPrice - earliestPrice) / numObs;
          } else {
              _movingAverage -= (earliestPrice - currentPrice) / numObs;
          }

          // Push new observation into storage and store timestamp taken at
          /// @audit also unchecked addition
          /// @audit Cache the value of `nextObsIndex` to save an SLOAD
          uint32 cachedNextObsIndex = nextObsIndex;
          observations[cachedNextObsIndex] = currentPrice;
          lastObservationTime = uint48(block.timestamp);
          nextObsIndex = (cachedNextObsIndex + 1) % numObs;
        }

Avoidable Second STATICCALL - 100+ gas

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/Kernel.sol#L238-L239

            ensureValidKeycode(Module(target_).KEYCODE());

Can instead cache keycode = Module(target_).KEYCODE(); and pass it to the next function `installModule(target, keycode);

Saving over 100 gas (STATICCALL + cost of processing the string for the return value)

Cache Storage Var - Save 100 gas

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L165-L166

            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))

Cache the value of observationFrequency to save 100 gas

Free Unchecked - 80+ gas

You can wrap the code below in unchecked to gain around 80 gas;

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/INSTR.sol#L44-L45

        uint256 instructionsId = ++totalInstructions;

Usual Suspects

Cache length - 3 gas

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

        for (uint256 step; step < instructions.length; ) {