OlympusDAO Bophades Remediation Audit First Phase

This audit was conducted by kebabsec members sai, FlameHorizon and okkothejawa and it is concerned with the validating remediations done for the issues detailed in the code4rena report and the commits in Remediation pull request.

All credits for the vulnerabilities detailed in the first section go to the people who found them and to code4rena, this part of the audit is simply to verify mitigation changes done by the OlympusDAO team.

Issues relating to governance was left out of scope for the remediation audit due to request from the team and they will be covered in second phase of the audit.

This report consists of two parts, the first part is a brief overview of the remediation status of issues described in the code4rena. In this section we used five keywords to describe remediation status of each issue:

Fixed: Issues that are completely fixed Acknowledged: Issues that the team left as is deliberately Partial fix: Issues that were not completely fixed and still need attention Invalid fix: Issues that were patched in a way resulting in new bugs Out of scope: Issue is about governance thus out of scope

The second part of the report is about the issues we found during the examination of the commits in Remediation pull request starting from commit 13fa439 and ending with commit 5072e5e.

This report does not provide any guarantee or warranty of the security of the project.

REVIEW OF C4 ISSUES

HIGH SEVERITY FINDINGS

H-01 In Governance.sol, it might be impossible to activate a new proposal forever after failed to execute the previous active proposal. Status: Out of Scope

H-02 Anyone can pass any proposal alone before first VOTES are minted Status: Out of Scope

H-03 TRSRY: front-runnable setApprovalFor Status: Fixed

Fixed with increase/decreaseAllowance pattern in https://github.com/OlympusDAO/bophades2/pull/73/commits/13fa439014df244bbedee947233af26f96ab26ad.

MEDIUM SEVERITY FINDINGS

M-01 Operator::setReserveFactor doesn't check if bond market should be changed Status: Acknowledged

M-02 Solmate safetransfer and safetransferfrom does not check the codesize of the token address, which may lead to fund loss Status: Partial fix

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/271c64650b8e8741869399812309b6f0cf7198ab, but see [KS-06].

M-03 RBS may redeploy funds automatically if price stays above or below wall for longer than _config.regenWait Status: Partial fix

See [KS-09].

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/fd1bca07b1ab55cc84e3287b89d980b5ed273628.

M-04 OlympusGovernance#executeProposal: reentrancy attack vulnerable function Status: Out of Scope

M-05 Proposals overwrite Status: Out of Scope

M-06 After endorsing a proposal, user can transfer votes to another user for endorsing the same proposal again Status: Out of Scope

M-07 Endorsed votes by a user do not decrease after the user's votes are revoked Status: Out of Scope

M-08 "TWAP" used is an observation-weighted-average-price, not a time-weighted one Status: Acknowledged

M-09 activateProposal() need time delay Status: Out of Scope

M-10 Voted votes cannot change after the user is issued new votes or the user's old votes are revoked during voting Status: Out of Scope

M-11 OlympusGovernance: Users can prevent their votes from being revoked Status: Out of Scope

M-12 Griefing/DOS of withdrawals by EOAs from treasury (TRSRY) possible Status: Not fixed

This issue persists in the codebase in the develop branch, see [KS-07] and fix by locking the function revokePolicyApprovals behind access control. If EOAs/multisigs were to be approved by treasury, this issue may lead to critical griefing attacks.

M-13 Missing checks in Kernel._deactivatePolicy Status: Invalid fix

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/7792e68ff8df979275506d2b0a296ce5500e6680, yet the commit fails to resolve the issue as the necessary logical condition is inverted wrongly, see [KS-03].

M-14 The governance system can be held hostage by a malicious user Status: Out of Scope

M-15 Heart will stop if all rewards are swept Status: Fixed

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/92a3a5cbbe38cfe4e4f97134c6e4e00f5753eea2.

M-16 Inconsistent parameter requirements between constructor() and Set() functions in RANGE.sol and Operator.sol. Status: Fixed

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/e14901a93735a4a3fff423b63d47ec92dba1f697.

M-17 No Cap on Amount of VOTES means the voter_admin can get any proposal to pass Status: Out of Scope

M-18 Inconsistency in staleness checks between OHM and reserve token oracles Status: Invalid fix

The issue is not resolved as the updating thresholds can still be inconsistent depending on the constructor parameters, see [KS-08].

M-19 TRSRY: reenter from OlympusTreasury::repayLoan to Operator::swap Status: Acknowledged

Addressed in https://github.com/OlympusDAO/bophades2/pull/73/commits/8a442ba9de331b056d5d6fbe2b612d9e55665ad0 by adding comments.

M-20 Operator: if WallSpread is 10000, operate and beat will revert and price information cannot be updated anymore Status: Fixed

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/4ccb52f2f788a284afef76ecc1489f5cb7748886.

M-21 OlympusGovernance - active proposal does not expire Status: Out of Scope

M-22 Low market bonds/swaps not working after loan is taken from treasury Status: Not fixed

We are a bit confused of this report, but from our understanding the issue is related to the fact that it may not be possible to redeem bonds in the case treasury is almost drained with a large loan. Yet, treasury doesn't implement any checks regarding reserve requirements in the debt function incurDebt, so we believe this issue was not addressed.

M-23 Treasury module is vulnerable to cross-contract reentrancy Status: Fixed with negligible side effect

See [KS-05].

M-24 Chainlink's latestRoundData Might Return Stale Results Status: Partial fix

The sanity checks on comments are not exactly implemented in the code, see [KS-01].

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/34df014792ab5211b10dc7ab7db7190fbc1c85de.

M-25 Moving average precision is lost Status: Fixed

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/5072e5e6b1892c5099547aaf8dc6ac08acea9395.

M-26 Cushion bond markets are opened at wall price rather than current price Status: Fixed

M-27 Unexecutable proposals when Actions.MigrateKernel is not last instruction Status: Not fixed

The checks that makes sure MigrateKernel is not the last instruction which are detailed in the report of [M-27] are not implemented.

M-28 Activating same Policy multiple times in Kernel possible Status: Invalid fix

Like [M-13], the issue is not resolved as AND operator is used instead of OR operator.

M-29 TRSRY susceptible to loan / withdraw confusion Status: Fixed

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/d7a0bc743355b60bf414e3872c07e8009b6c57cc

M-30 Heart::beat() could be called several times in one block if no one called it for a some time Status: Fixed

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/870bd6ff74b11d35d350b40314ac43364e6c5095

M-31 Protocol's Walls / cushion bonds remain active even if heart is not beating Status: Fixed

Addressed with onlyWhileActive modifier: https://github.com/OlympusDAO/bophades2/blob/develop/src/policies/Operator.sol#L212

M-32 Admin cannot be changed to EOA after deployment Status: Not fixed

Issue still persists as store() still enforces instruction targets to be contracts in https://github.com/OlympusDAO/bophades2/blob/develop/src/modules/INSTR/OlympusInstructions.sol#L56 and as executor still cannot be changed to an EOA.

It should be noted that as admin is removed, the problem persists only with executor, yet still valid.

LOW AND NON-CRITICAL SEVERITY FINDINGS

L-01 Operator: incorrect accounting for fee-on-transfer reserve token Status: Not fixed Not resolved.

L-02 BondCallback: incorrect accounting if quoteToken is rebase token Status: Not fixed Not resolved.

L-03 PRICE: unsafe cast for numObservations Status: Acknowledged No fix applied, yet we think its unrealistic for numObservations to overflow uint32, as it would necessiate a long period of moving average duration and too frequent observations.

It is recommended to use SafeCast library when such casts can overflow.

L-04 Operator: unsafe cast for decimals Status: Acknowledged No fix applied, the int8 to uint8 cast can underflow but not probable.

It is recommended to use SafeCast library when such casts can overflow.

L-05 BondCallback: operator is not set constructor Status: Not fixed

No changes applied. Operator is still unset after deploy, which makes functions like callback revert.

Remediated version still isn’t set at constructor time so L153 of BondCallback will fail.

L-06 Operator: missing check for configParams[0] (cushionFactor) in the constructor Status: Fixed

Resolved by appending a check for configParams[0] in constructor

L-07 Misplaced zero address check for changeKernel Status: Not fixed

changeKernel is left unchanged.

L-08 BondCallback, Operator: upon module's upgrade, the token approval should be revoked Status: Not fixed

Currently there’s no mitigation.

BondCall, Operator approve ohm to the MINTR module, but there’s no logic to be able to revoke it in case there’s bugs and there’s the need to an emergency revoke.

L-09 Heart: if the issueReward fails the heart beat will revert Status: Fixed

Solved by the patch made to [M-15] in https://github.com/OlympusDAO/bophades2/pull/73/commits/92a3a5cbbe38cfe4e4f97134c6e4e00f5753eea2.

N-01 Kernel: missing zero address check for executor and admin Status: Not fixed

No fix applied. admin has been removed, executor is left unchanged.

N-02 INSTR, Governance: upon module's upgrade, all instruction data should be carried over to the new modules Status: Out of Scope

N-03 RANGE, PRICE: unused import of FullMath Status: Fixed

Addressed in: https://github.com/OlympusDAO/bophades2/pull/73/commits/f2e263356ae8b0bb3c73b7b730aa59af6fe78b29.

N-04 PRICE: stale price Status: Acknowledged

We believe that this issue is mitigated as much as possible with staleness checks but see [KS-01] and [KS-08].

ISSUES FOUND IN REMEDIATION

[KS-01][L] Staleness sanity check is not implemented in line with documentation

In lines 218 and 228 of OlympusPrice.sol, it is stated that answeredInRound should be same with roundId yet the code only checks if

1
answeredInRound < roundId

even though the comment states

1
// 3. Answered in round ID should be the same as the round ID

Mitigation: The line should be changed to:

1
answeredInRound != roundId

[KS-02][M] No functions to decrease allowance partially in `TreasuryCustodian.sol`

Before C4 audit (master branch) it was possible for the Custodian to decrease an address's allowance of the treasury by calling grantApproval with a lower amount than the current one. But as now the approvals are controlled through the increase/decreaseAllowance pattern, TreasuryCustodian lacks a function to decrease an address's allowance (in contrast, it has grantWithdrawerApproval and grantDebtorApproval, which are calling TRSRY's approval incrementing functions.)

Mitigation: Add functions that can decrease allowance.

[KS-03][M] Adding of extra sanity checks to `_activatePolicy` and `_deactivatePolicy` of Kernel.sol is not implemented correctly

In lines 279 and 310 of Kernel.sol, two checks (one initial check and one added after the audit) are connected via an AND operator, but an OR operator should be used as in this context, the checks are used in a custom error pattern and thus the code fails if the both checks return true. Connecting them via an AND operator results in a fewer amount of faulty cases instead of a higher amount of faulty cases which is what was actually intended.

Mitigation: Use an OR operator to connect checks.

[KS-04][L] `withdrawReserves` of TRSRY might be misused to fire fake events

The function withdrawReserves is permissionless, while this does not oppose a threat, it allows any user to fire events by inputting amount_ as 0, while having approval also as 0, which passes the revert statement, firing the Withdrawal event.

Mitigation: Check amount_ != 0 for a trivial fix, it shouldn't create a problem as withdrawing 0 amount of tokens is against usual use case. This check also implicitly enforces the msg.sender to have explicit approval from TRSRY for that specific token as non-zero amount_ necessiates non-zero approval.

[KS-05][NC] TRSRY may receive more than intended tokens through `repayDebt`

In repayDebt there's an if clause in line 140 that changes the received variable to amount_, which may cause the contract to receive more tokens than intended that would normally go to the user, while taking the difference for itself, rather than also going to repay the debt. This issue is a side-effect caused by the fix to the issue [M-23] and can be acknowledged with just a comment as its highly unlikely this would create a problem. (We don't know any tokens that transfers more to the receiver than what is sent by the sender, it would be the opposite case of a fee-on-transfer token.)

Mitigation: We have no trivial fix and we believe a comment is sufficient to address the issue.

[KS-06][G] Redundant `address(token).code.length` check in `safeTransferFrom` of `TransferHelper`

Contract TransferHelper.sol was changed to remediate issue #117, corresponding to [M-02], by adding address(token).code.length > 0 to the require statements ensuring that the transaction goes through. This is correct, but this require statement was placed twice in the same function, both in lines 16, and in line 25, making it unnecessary and redundant. We suggest the team to keep the check in line 16, as to match the rest of the code in TrasnferHelper.sol.

Mitigation: Remove the check starting in line 25.

[KS-07][M] Problems may arise if multisigs are approved by treasury due to `revokePolicyApprovals` in `TreasuryCustodian`

The function revokePolicyApprovals allows anyone to revoke a deactivated policy's approvals. Although this is perfectly fine, an issue could arise if a multisig is approved, as this function does not have a check to ensure only policies can be revoked. A malicious actor could grief the contract by constantly revoking previously intentionally approved addresses that are not policies. See [M-12] of C4 report.

Mitigation: Locking the function behind access control is the most trivial fix, but if the team is wishing to keep the permissionless structure, they may use a list of deactivated policies and check against it.

[KS-08][M] Staleness checks in `getCurrentPrice()` can be inconsistent

By C4 issue [M-18], it was decided that price freshness tolerance for both the ETH oracle and OHM oracle should be same, yet in this version of the codebase they are assigned through a constructor parameter and it is not checked that if they are the same, thus the contract is still prone to [M-18].

Mitigation: To mitigate, add a "ohmEthUpdateThreshold_ == reserveEthUpdateThreshold_" check to the constructor of OlympusPrice and add zero checks.

Additionally, consider adding permissioned setters for them to update later.

Also make sure that if the provided threshold is realistically long enough for the oracle to report data.

[KS-09][NC] Regen parameters of `Operator.sol` are not checked in constructor

In L104, a comment suggests a condition, ensured in setRegenParams but does not ensure in constructor time, which may invalidate the comment statement. See [M-03].

Mitigation: If possible, ensure the invariant in constructor time, if not, specify in comments and ensure parameters passed during deployment would satisfy the invariant.