Olympus DAO contest - datapunk's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L272-L289

Vulnerability details

Impact

RANGE constructor does not validate inputs, which may result in unexpected cushion/walls

Proof of Concept

RANGE.sol#L272-L289

Tools Used

Recommended Mitigation Steps

Add the following:

require(rangeParams[1]<rangeParams[2]) ;
require(rangeParams[0]>=100 && rangeParams[0]<=10000);
require(rangeParams[1]>=100 && rangeParams[1]<=10000);
require(rangeParams[2]>=100 && rangeParams[2]<=10000);

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L75 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L92

Vulnerability details

Impact

pricefeed has different cutoffs for ohm and reserve

Proof of Concept

TRSRY.sol#L75 TRSRY.sol#L92

Tools Used

Recommended Mitigation Steps

if this intentional, please explain rationale in comments. Otherwise, select a common cutoff.

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L75 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L92

Vulnerability details

Impact

TRSRY's withdrawApproval is used for two different purposes: to keep track of reserves for withdrawReserves and for loan taking in getLoan. In case a policy implements both functionalities, it would always do withdrawReserves to avoid incurring debt.

Proof of Concept

TRSRY.sol#L75 TRSRY.sol#L92

Tools Used

Recommended Mitigation Steps

it would be best if there is a different variable that keeps track of loans, which is different from reserve contribution.

Lines of code

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

Vulnerability details

Impact

Assuming 5 beats were skipped. lastBeat = block.timestamp - 5 * freq() This allows 5 beat() calls sequentially, while getCurrentPrice may return prices of the same moment , which the actual MA might have moved a lot

Proof of Concept

Heart.sol#L92

Tools Used

Recommended Mitigation Steps

interpolate the prices somehow from most recent available observations

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/INSTR.sol#L52

Vulnerability details

Impact

In kernel, Actions.ChangeExecutor/Actions.ChangeAdmin does not require ensureContract(instruction.target); while in INSTR, it is required.

Proof of Concept

INSTR.sol#L224-L228 Kernel.sol#L250-L253

Tools Used

Recommended Mitigation Steps

make them consistent in kernel and INSTR. My sense is to go with kernel, so that EOAs are allowed to be executor and admin

QA: repeating code can be extracted into a function. These two function can be merged, thus allowing more flexible updates of MADuration and ObsFreq, ie, their updated values would not be constrained current MADuration and ObsFreq, as remainder is required to be 0

PRICE.sol#L246-L257 PRICE.sol#L272-L289

GAS: can revert earlier to save gas

INSTR.sol#L224-L228

GAS: endorseProposal may be called after proposal.submissionTimestamp + ACTIVATION_DEADLINE, however at this point, the proposal can no longer be activated, so it is better to revert the endorse

Governance.sol#L180

QA: It does not look reducingBy 0 actually do anything in these two lines?

Heart.sol#L92

QA: it is unclear to me the difference between marketCapacity and capacity in Side.

updateMarket does not use marketCapacity in any way except for emit. RANGE.sol#L215

QA: missing emits in setters. For example:

Kernel.sol#L76 Kernel.sol#L126 BondCallback.sol#L190