Olympus DAO contest - Trust's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L64-L71

Vulnerability details

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

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L240

Vulnerability details

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.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L160-L174

Vulnerability details

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.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/TRSRY.sol#L64-L102

Vulnerability details

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

1. Policy P has getLoan permission
2. setApprovalFor(policy, token, amount) was called to grant P permission to loan amount
3. 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.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L92-L103

Vulnerability details

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, since 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 heart

lastBeat should be tracking block.timestamp to prevent the clock from going more and more out of sync.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L188-L191 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Operator.sol#L272 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L346

Vulnerability details

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 as usual.

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())

Operator's setRegenParams does some validation on the input:

        if (wait_ < 1 hours || threshold_ > observe_ || observe_ == 0)
            revert Operator_InvalidParams();

However, it must also check that threshold_ != 0, otherwise the system will keep regenerating

Operator's setThresholdFactor updates the current threshold used for determining if walls are depleted. However, this threshold will not take effect until after regenerate() is called, which may be in a long time, and before the new threshold is needed. Make sure to update the existing walls, if they are active:

if (_range.high.active == true) {
    uint256 threshold = (fullCapacity(true) * thresholdFactor) / FACTOR_SCALE;
    _range.high.threshold = threshold
}
if (_range.low.active == true) {
    uint256 threshold = (fullCapacity(false) * thresholdFactor) / FACTOR_SCALE;
    _range.low.threshold = threshold
}