Olympus DAO contest - d3e4's results

Lines of code

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

Vulnerability details

Impact

With only ENDORSEMENT_THRESHOLD% (currently 20%) voting power, a malicious user can prevent any other proposal from being activated. While ENDORSEMENT_THRESHOLD is currently fairly high, it seems not higher than that it might not be used to hold the system hostage to extract far more funds.

Proof of Concept

Submit a dummy proposal, endorse it and then activate it. Now, no other proposal can be activated for a GRACE_PERIOD. When this time period is over, this procedure may be repeated, either immediately or just before any other proposal activation by front-running.

Tools Used

Code inspection

Recommended Mitigation Steps

Making sure ENDORSEMENT_THRESHOLD is at least 50% seems discouraging enough. Other more creative solutions should be possible. One might be to let the most endorsed proposal be activated, or restricting who can activate a proposal; anything that at least temporarily liberates the governance system so that the attacker is dissuaded from investing in this attack method.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L36 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L122-L147 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L135-L139 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L92-L109

Vulnerability details

Impact

The moving average is critical for the RBS-system. Its current calculation allows for compounding drift, randomly as well as maliciously, detaching from the true value, which invalidates the entire system, including affecting the way funds are handled.

Proof of Concept

The moving average is not directly calculated but is updated in differential increments. This allows for drift when the calculations are inaccurate. The moving average is calculated in updateMovingAverage() by adding the difference of the value to be added and the value to be removed from the average, scaled to the number of data points:

if (currentPrice > earliestPrice) {
            _movingAverage += (currentPrice - earliestPrice) / numObs;
        } else {
            _movingAverage -= (earliestPrice - currentPrice) / numObs;
        }

The division by numObs, proposed as 360 per the documentation, floors the change in the moving average at each heart beat by up to 359 each time. Thus the moving average increases too little when it goes up (compared to the first observation in the window) and decreases too little when it goes down. The significance of this is higher the cheaper OHM is compared to the reserve asset (such that we lose effective decimals).

For example, if the price is increasing very slowly, the moving average will remain stationary. But more generally, the discrepancy between the added and subtracted errors due to flooring can be artificially manipulated by manipulating the price within only 360 price points. By causing a greater error in one direction the moving average can be caused to slowly drift as desired. This is facilitated by the fact that anyone can call beat() in Heart.sol to trigger the updating of the moving average.

Tools Used

Code inspection

Recommended Mitigation Steps

Instead of updating the moving average itself, the sum total of the observations in the moving window should be updated and used as the stored record. Thus no rounding is done and an exact record is kept (up to only the inaccuracies in the observations themselves). This sum can then be scaled by dividing by the number of observations when needed as the average.

executeProposal() will revert prematurely when there are more no-votes than yes-votes uint256 netVotes will revert due to underflow if there are more no-votes than yes-votes. It should revert with the check just after it. Consider declaring it as a signed integer instead.

Magic numbers Consider defining constants instead of using magic numbers.

Instances: RANGE.sol: 246 248 264 PRICE.sol: 90 Governance.sol: 217 268 Operator.sol: 106 108 111 114 378 433 518 533 535 536 550 565

Typos

Instances: PRICE.sol: “numbe” should be “number” 126

Incorrect comment /// @dev This amount should be greater than 0 to prevent flash loan attacks. This is incorrect strictly as written. It must be greater than 0 in days, or more specifically it must be greater than the time of one block.

Pre-incrementing is cheaper than post-incrementing Consider replacing e.g. i++ with ++i.

Instances: KernelUtils.sol: 49 64

Function not called by the contract can be external instead of public

Instances: Kernel.sol: 95 100 439 451 TRSRY.sol: 47 79 MINTR.sol: 20 25 33 37 RANGE.sol: 110 115 219 PRICE.sol: 108 113 VOTES.sol: 22 27 45 55 INSTR.sol: 23 28 37 Governance.sol: 145 151

Pre-compute .length before repeated usage, especially in loops

Instances: Governance.sol#L278

observations.length is already computed in numObservations uint256 numObs = observations.length;can be changed into uint256 numObs = numObservations; as suggested by its preceding comment, because its length is already calculated during construction. PRICE.sol#L212