or
or
By clicking below, you agree to our terms of service.
New to HackMD? Sign up
Syntax | Example | Reference | |
---|---|---|---|
# Header | Header | 基本排版 | |
- Unordered List |
|
||
1. Ordered List |
|
||
- [ ] Todo List |
|
||
> Blockquote | Blockquote |
||
**Bold font** | Bold font | ||
*Italics font* | Italics font | ||
~~Strikethrough~~ | |||
19^th^ | 19th | ||
H~2~O | H2O | ||
++Inserted text++ | Inserted text | ||
==Marked text== | Marked text | ||
[link text](https:// "title") | Link | ||
![image alt](https:// "title") | Image | ||
`Code` | Code |
在筆記中貼入程式碼 | |
```javascript var i = 0; ``` |
|
||
:smile: | Emoji list | ||
{%youtube youtube_id %} | Externals | ||
$L^aT_eX$ | LaTeX | ||
:::info This is a alert area. ::: |
This is a alert area. |
On a scale of 0-10, how likely is it that you would recommend HackMD to your friends, family or business associates?
Please give us some advice and help us improve HackMD.
Syncing
xxxxxxxxxx
Bophades Remediation Audit
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 commit13fa439
and ending with commit5072e5e
.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 inhttps://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 functionrevokePolicyApprovals
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
toOperator::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
andbeat
will revert and price information cannot be updated anymoreStatus: 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 instructionStatus: 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 timeStatus: 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 asexecutor
still cannot be changed to an EOA.It should be noted that as
admin
is removed, the problem persists only withexecutor
, 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]
inconstructor
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 thatansweredInRound
should be same withroundId
yet the code only checks ifeven though the comment states
Mitigation: The line should be changed to:
[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 theincrease/decreaseAllowance
pattern,TreasuryCustodian
lacks a function to decrease an address's allowance (in contrast, it hasgrantWithdrawerApproval
andgrantDebtorApproval
, 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 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.
[KS-04][L]
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 inputtingamount_
as 0, while havingapproval
also as 0, which passes the revert statement, firing theWithdrawal
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 themsg.sender
to have explicit approval from TRSRY for that specific token as non-zeroamount_
necessiates non-zeroapproval
.[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 toamount_
, 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 insafeTransferFrom
ofTransferHelper
Contract
TransferHelper.sol
was changed to remediate issue #117, corresponding to [M-02], by addingaddress(token).code.length > 0
to therequire
statements ensuring that the transaction goes through. This is correct, but thisrequire
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 inTrasnferHelper.sol
.Mitigation: Remove the check starting in line 25.
[KS-07][M] Problems may arise if multisigs are approved by treasury due to
revokePolicyApprovals
inTreasuryCustodian
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 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 ofOlympusPrice
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 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.