Olympus DAO contest - 0xDjango's results

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] DOS of grantApproval()

The fact anyone can revoke approval without delay or time lock provides an easy way to DOS the granting of approval to other addresses. This issue is already addressed in TODO below, but should not be overlooked as sometimes people make mistakes. 1 instance of this issue has been found:

[L-01] TreasuryCustodian.sol#L50-L67

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

        uint256 len = tokens_.length;
        for (uint256 j; j < len; ) {
            TRSRY.setApprovalFor(policy_, tokens_[j], 0);
            unchecked {
                ++j;
            }
        }

        emit ApprovalRevoked(policy_, tokens_);
    }

[L-02] Critical function should return bool

The caller of such function should be able to check the return value to ensure the call was successful. 5 instances of this issue have been found:

[L-02] TRSRY.sol#L137-L152

    function _checkApproval(
        address withdrawer_,
        ERC20 token_,
        uint256 amount_
    ) internal {
        // Must be approved
        uint256 approval = withdrawApproval[withdrawer_][token_];
        if (approval < amount_) revert TRSRY_NotApproved();

        // Check for infinite approval
        if (approval != type(uint256).max) {
            unchecked {
                withdrawApproval[withdrawer_][token_] = approval - amount_;
            }
        }
    }

[L-02b] TRSRY.sol#L122-L135

    function setDebt(
        ERC20 token_,
        address debtor_,
        uint256 amount_
    ) external permissioned {
        uint256 oldDebt = reserveDebt[token_][debtor_];

        reserveDebt[token_][debtor_] = amount_;

        if (oldDebt < amount_) totalDebt[token_] += amount_ - oldDebt;
        else totalDebt[token_] -= oldDebt - amount_;

        emit DebtSet(token_, debtor_, amount_);
    }

[L-02c] TRSRY.sol#L105-L119

    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_);

        uint256 received = token_.balanceOf(address(this)) - prevBalance;

        // Subtract debt from caller
        reserveDebt[token_][msg.sender] -= received;
        totalDebt[token_] -= received;

        emit DebtRepaid(token_, msg.sender, received);
    }

[L-02d] TRSRY.sol#L75-L85

    function withdrawReserves(
        address to_,
        ERC20 token_,
        uint256 amount_
    ) public {
        _checkApproval(msg.sender, token_, amount_);

        token_.safeTransfer(to_, amount_);

        emit Withdrawal(msg.sender, to_, token_, amount_);
    }

[L-02e] TRSRY.sol#L92-L102

    function getLoan(ERC20 token_, uint256 amount_) external permissioned {
        _checkApproval(msg.sender, token_, amount_);

        // Add debt to caller
        reserveDebt[token_][msg.sender] += amount_;
        totalDebt[token_] += amount_;

        token_.safeTransfer(msg.sender, amount_);

        emit DebtIncurred(token_, msg.sender, amount_);
    }

[L-03] Missing check if address is a contract

No check to ensure auctioneer_ and callback_ are contract address and not EOAs. 1 instance of this issue has been found:

[L-03] Operator.sol#L586-L595

    function setBondContracts(IBondAuctioneer auctioneer_, IBondCallback callback_)
        external
        onlyRole("operator_admin")
    {
        if (address(auctioneer_) == address(0) || address(callback_) == address(0))
            revert Operator_InvalidParams();
        /// Set contracts
        auctioneer = auctioneer_;
        callback = callback_;
    }

[L-04] safeApprove has been deprecated deprecated

Please use safeIncreaseAllowance and safeDecreaseAllowance instead. 1 instance of this issue has been found:

[L-04] BondCallback.sol#L57-L58

        ohm.safeApprove(address(MINTR), type(uint256).max);

[L-05] Unsafe transferfrom()

Check the return value to ensure the transfer was successful. 2 instances of this issue have been found:

[L-05] Governance.sol#L259-L260

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

[L-05b] Governance.sol#L312-L313

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

[L-06] executor transfer should be a two-step process

If ownership is accidentally transferred to an inactive account all functionalities depending on it will be lost. 1 instance of this issue has been found:

[L-06] Kernel.sol#L250-L251

else if (action_ == Actions.ChangeExecutor) {
            executor = target_;

[L-07] Missing zero address check

Could render kernel unusable. 1 instance of this issue has been found:

[L-07] Kernel.sol#L250-L251

else if (action_ == Actions.ChangeExecutor) {
            executor = target_;

Non-critical Findings

[N-01] Remove TODOs

They add unnecessary cluttler and harm readbility for auditors. 3 instances of this issue have been found:

[N-01] Operator.sol#L657-L658

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

[N-01b] TreasuryCustodian.sol#L56-L57

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

[N-01c] TreasuryCustodian.sol#L51-L52

// TODO Currently allows anyone to revoke any approval EXCEPT activated policies.
// TODO must reorg policy storage to be able to check for deactivated policies.

[N-02] It is recommend to use scientific notation (1e18) instead of exponential (10**18)

Improves readbility. 1 instance of this issue has been found:

[N-02] PRICE.sol#L91-L92

        _scaleFactor = 10**exponent;

[N-03] The use of magic numbers is not recommended

Consider setting constant numbers as a constant variable for better readability and clarity. 13 instances of this issue have been found:

[N-03] Operator.sol#L550-L551

        if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams();

[N-03b] Operator.sol#L535-L536

        if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams();

[N-03c] Operator.sol#L518-L519

        if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();

[N-03d] Governance.sol#L268-L269

        if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {

[N-03e] Governance.sol#L217-L218

            (totalEndorsementsForProposal[proposalId_] * 100) <

[N-03f] Governance.sol#L164-L165

        if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)

[N-03g] Operator.sol#L550-L551

        if (reserveFactor_ > 10000 || reserveFactor_ < 100) revert Operator_InvalidParams();

[N-03h] Operator.sol#L535-L536

        if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams();

[N-03i] Operator.sol#L111-L112

        if (configParams[4] > 10000 || configParams[4] < 100) revert Operator_InvalidParams();

[N-03j] Operator.sol#L106-L107

        if (configParams[2] < uint32(10_000)) revert Operator_InvalidParams();

[N-03k] PRICE.sol#L90-L91

        if (exponent > 38) revert Price_InvalidParams();

[N-03l] RANGE.sol#L264-L265

        if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();

[N-03m] RANGE.sol#L245-L248

            wallSpread_ > 10000 ||
            wallSpread_ < 100 ||
            cushionSpread_ > 10000 ||
            cushionSpread_ < 100 ||

[N-04] Unindexed parameters in events

Events should index all existing parameters (up to three) to facilitate access to off-chain tools that are parsing events. Worth noting every indexed event costs extra gas, so is up to the project to assess the trade-off. 6 instances of this issue have been found:

[N-04] Governance.sol#L86-L90

    event ProposalSubmitted(uint256 proposalId);
    event ProposalEndorsed(uint256 proposalId, address voter, uint256 amount);
    event ProposalActivated(uint256 proposalId, uint256 timestamp);
    event WalletVoted(uint256 proposalId, address voter, bool for_, uint256 userVotes);
    event ProposalExecuted(uint256 proposalId);

[N-04b] Operator.sol#L45-L54

event Swap(
        ERC20 indexed tokenIn_,
        ERC20 indexed tokenOut_,
        uint256 amountIn_,
        uint256 amountOut_
    );
    event CushionFactorChanged(uint32 cushionFactor_);
    event CushionParamsChanged(uint32 duration_, uint32 debtBuffer_, uint32 depositInterval_);
    event ReserveFactorChanged(uint32 reserveFactor_);
    event RegenParamsChanged(uint32 wait_, uint32 threshold_, uint32 observe_);

[N-04c] Heart.sol#L28-L30

    event Beat(uint256 timestamp_);
    event RewardIssued(address to_, uint256 rewardAmount_);
    event RewardUpdated(ERC20 token_, uint256 rewardAmount_);

[N-04d] RANGE.sol#L20-L31

    event WallUp(bool high_, uint256 timestamp_, uint256 capacity_);
    event WallDown(bool high_, uint256 timestamp_, uint256 capacity_);
    event CushionUp(bool high_, uint256 timestamp_, uint256 capacity_);
    event CushionDown(bool high_, uint256 timestamp_);
    event PricesChanged(
        uint256 wallLowPrice_,
        uint256 cushionLowPrice_,
        uint256 cushionHighPrice_,
        uint256 wallHighPrice_
    );
    event SpreadsChanged(uint256 cushionSpread_, uint256 wallSpread_);
    event ThresholdFactorChanged(uint256 thresholdFactor_);

[N-04e] TRSRY.sol#L27-L29

    event DebtIncurred(ERC20 indexed token_, address indexed policy_, uint256 amount_);
    event DebtRepaid(ERC20 indexed token_, address indexed policy_, uint256 amount_);
    event DebtSet(ERC20 indexed token_, address indexed policy_, uint256 amount_);

[N-04f] TRSRY.sol#L20-L21

    event ApprovedForWithdrawal(address indexed policy_, ERC20 indexed token_, uint256 amount_);

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operatio 1 instance of this issue has been found:

[G-01] TRSRY.sol#L33-L39

    mapping(address => mapping(ERC20 => uint256)) public withdrawApproval;

    /// @notice Total debt for token across all withdrawals.
    mapping(ERC20 => uint256) public totalDebt;

    /// @notice Debt for particular token and debtor address
    mapping(ERC20 => mapping(address => uint256)) public reserveDebt;

[G-02] Unnecessary init to 0

These values are set by default. 1 instance of this issue has been found:

[G-02] Kernel.sol#L397-L398

        for (uint256 i = 0; i < reqLength; ) {

[G-03] bool is gas inefficient when used in storage

Instead use uint256 values to represent true/false instead. Reference 2 instances of this issue have been found:

[G-03] Operator.sol#L62-L66

    /// @notice    Whether the Operator has been initialized
    bool public initialized;

    /// @notice    Whether the Operator is active
    bool public active;

[G-03b] Kernel.sol#L113-L114

    bool public isActive;

[G-04] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

1 instance of this issue has been found:

[G-04] Governance.sol#L278

instructions

[G-05] use transfer() instead of transferFrom()

There is no need to use transferFrom() to transfer money INTO an account. 1 instance of this issue has been found:

[G-05] Governance.sol#L312-L313

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