I've competed in this contest between 25/08/22-01/09/22 and achieved third place. Olympus DAO is the governance mechanism behind Olympus OHM token. Below are my accepted findings.
HIGH: 1
MED: 5
HIGH: TRSRY grants withdraw approval unsafely
Description
setApprovalFor() is used to grant permission for some address to withdraw X amount of funds. withdrawReserves() can later be directly called to withdraw the approved amount. However, if previous approval was X and setApprovalFor(address, token, Y) is called, a total of X+Y can be withdrawn , although the intention is for a maximum of Y to be withdrawn. This is because attacker can frontrun the setApprovalFor(address, token, Y) call with a withdrawReserves(address, token, X) call and therefore empty the existing approval before it is replenished. This kind of attack is well known, for example ERC20 increaseAllowance() function is used instead of approve() for this reason.
Impact
Attacker may withdraw X+Y amount instead of X or Y from the treasury.
Proof of Concept
1. setApprovalFor(attacker, token, X) called
2. setApprovalFor(attacker, token, Y) transaction is in mempool
3. Attacker front-runs with withdrawReserves(address, token, X) transaction, which is executed first
4. setApprovalFor(attacker, token, Y) is executed
5. withdrawReserves(address, token, Y) is called
6. Attacker withdraws a total of X+Y tokens
Recommended Mitigation Steps
Refactor the affected code to increaseApprovalFor(Y), which checks existing approval, and increases it by Y amount
MED: TRSRY susceptible to loan / withdraw confusion
Impact
Treasury allocates approvals in the withdrawApproval mapping which is set via setApprovalFor(). In both withdrawReserves() and in getLoan(), _checkApproval() is used to verify user has enough approval and subtracts the withdraw / loan amount. Therefore, there is no differentiation in validation between loan approval and withdraw approval. Policies which will use getLoan() (currently none) can simply withdraw the tokens without bookkeeping it as a loan.
Proof of Concept
Policy P has getLoan permission
setApprovalFor(policy, token, amount) was called to grant P permission to loan amount
P calls withdrawReserves(address, token, amount) and directly withdraws the funds without registering as loan
Recommended Mitigation Steps
A separate mapping called loanApproval should be implemented, and setLoanApprovalFor() will set it, getLoan() will reduce loanApproval balance
MED: It's possible to steal votes between proposals
Explanation
When a proposal has over ENDORSEMENT_THRESHOLD % of votes, it may be activated using activateProposal() call. The current proposal must have been active for at least GRACE_PERIOD. To vote, users call vote(for_) with YES or NO for the current proposal, which sends their vote tokens to Governance and marks their vote in the vote table.
The vulnerability is that a proposal submitter can activate his proposal and override the current proposal, just before the vote() transaction is executed. By doing this, the victim casts his vote on the wrong proposal. It is not possible to claim back the misplaced vote tokens.
Impact
The main exploit scenario is that attacker waits for a whale to cast a YES vote on some proposal, and then frontrun to register it as a YES for his own proposal. Any manipulation of governance votes is always considered maximum severity because it undermines the core principal of majority rule.
Proof of Concept
1. User submits proposal P1 - submitProposal(instructions, title, URI)
2. User receives endorsements above ENDORSEMENT_THRESHOLD - endorseProposal(P1)
3. User activates his proposal - activateProposal(P1)
4. Attacker submits a proposal P2 - submitProposal(instructions2, title2, URI2)
5. Attacker receives endorsements above ENDORSEMENT_THRESHOLD - endorseProposal(P2)
6. Over GRACE_PERIOD of time (1 week) has passed since #3, and P1 has not been executed
7. Victim whale casts YES vote for P1
8. Attacker frontruns with activateProposal(P2)
9. Victim's YES vote is registered for P2
10. Attacker might be able to execute his proposal using the whale's votes - executeProposal()
Recommended Mitigation Steps
Vote() function MUST have a proposal_id parameter, just like any other function in the Governance policy.
MED: Protocol heart may stop beating due to dependency on oracle
Description
Heart's beat() function is extremely important as it keeps the protocol updated and reactive to market conditions (prices + RBS). It calls updateMovingAverage() to update the ETH/RESERVE moving average price, which in turn calls getCurrentPrice() to get the current price. If the oracle data is found to be stale, the function reverts, which makes sense to some degree as we could not determine a current price. However, as is documented in line 160, the oracle might not update if the data didn't change much, which is why it is good enough that the last update time is 3 times the observationFrequency. However, this relaxation of the update time is only used in the _ohmEthPriceFeed oracle and not in _reserveEthPriceFeed ( line #169). Therefore, if the reserve asset's price did not change much, which is likely with stablecoins, getCurrentPrice() reverts and the RBS is not reactive.
Impact
It is likely that at different points in the protocol's lifetime the heart beat() function will stop working, causing the RBS system to fail and the moving average to be completely outdated. Note that the fact reserve/ETH price did not change much does not indicate at all that the OHM/reserve moving average did not change as well.
Proof of Concept
1. observationFrequency = 4 hours
2. reserveEthPriceFeed tracks USDC price
3. USDC price did not change for 4 hours
4. Protocol keeper calls beat() but transaction is reverted.
5. Protocol does not track prices and RBS system is stale. This is a financial risk as cushion bonds will not be activated and more importantly, walls are not taken down if the price breached them upwards or downwards causing the protocol to lose money on incorrect trading.
Recommended Mitigation Steps
Use the same 3X factor of observationFrequency to validate the last update time of the reserveEthPriceFeed oracle.
MED: Heart beat could lose sync, making it possible to manipulate MA
Description
Heart beat() should be called around every observationFrequency (named F) time for the RBS to function. In order for beat() to succeed, at least F seconds have to pass since lastBeat. However, lastBeat is always increased by F and not actual seconds that have passed, the lastBeat will eventually go out of sync. The effect is that after some time of operation, or if beat() for some reason was not called, it is possible to call beat() multiple times in quick succession. Since beat() updates the moving average using currentPrice(), attacker is able to skew the MA , putting too much weight on the current price.
Impact
The RBS system may malfunction and very bizzare behavior can occur. For example, if there is a trend of downward movement in the last 5 minutes and beat is called several times, the operator may regenerate the high wall incorrectly. Another possible impact - operator might deactivate a cushion bond because it detects from the wrong MA that the price is no longer in the danger zone. Most severe impact - the zero slippage walls will defend the wrong price range. This means if the price strays up or down much less than the intended spread, the walls may take a big hit.
Proof of Concept
Attached a diagram in the references below.
References
https://imgur.com/a/k2PVncz - Diagram of proper RBS functioning and the MA attack https://docs.google.com/document/d/1AdPex_lMpSC_3U8UEU4hiSZIT1O1FekoCujRtYEJ0ig - documentation of the RBS system
Recommended mitigation steps
Add a check in beat() function:
if (block.timestamp > lastBeat + FREQ_PRECISION_THRESHOLD * frequency())
revert Heart_OutOfCycle(); // Too far out of sync, require admin to reset the heartlastBeat should be tracking block.timestamp to prevent the clock from going more and more out of sync.
MED: Protocol's Walls / cushion bonds remain active even if heart is not beating
Description
The Walls of the RBS mechanism offer zero slippage swaps at the high and low of the moving average spread. The capacity to be swapped at these prices is usually very large, so it must make sure to only be enabled when the prices are guaranteed to be synced. However, there is no such check. If beat() is not called for some time, meaning we cannot determine if the current spread is legit, swap() still operates fine.
Impact
The worst case scenario is that the wall is swapping at a losing price, meaning they can be immediately drained via arbitrage bot.
Proof of concept
1. Price is X at the start
2. Oracle stops updating for some reason / no one calls beat()
3. Price drops to Y , where Y < low wall centered around X
4. Attacker can perform arbitrage by buying Ohm at external markets at Y and selling Ohm at low wall price, netting the difference.
Recommended mitigation steps:
Change modifier onlyWhileActive to add a check for beat out of sync:
if (block.timestamp > lastBeat + SYNC_THRESHOLD * frequency())
Komentarze