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.
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.
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.
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-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.
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].
In lines 218 and 228 of OlympusPrice.sol, it is stated that answeredInRound should be same with roundId yet the code only checks if
answeredInRound < roundId
even though the comment states
// 3. Answered in round ID should be the same as the round ID
Mitigation: The line should be changed to:
answeredInRound != roundId
TreasuryCustodian.solBefore 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.
_activatePolicy and _deactivatePolicy of Kernel.sol is not implemented correctlyIn 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.
withdrawReserves of TRSRY might be misused to fire fake eventsThe 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.
repayDebtIn 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.
address(token).code.length check in safeTransferFrom of TransferHelperContract 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.
revokePolicyApprovals in TreasuryCustodianThe 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.
getCurrentPrice() can be inconsistentBy 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.
Operator.sol are not checked in constructorIn 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.
-
Any changes
Be notified of any changes
-
Mention me
Be notified of mention me
-
Unsubscribe
Subscribe