search

code-423n4 / 2021-10-tracer-findings

0 stars 0 forks source link

Revert in `poolUpkeep` #13

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

As PoolKeeper.performUpkeepSinglePool can handle reverts in pool.poolUpkeep, the LeveragedPool.executePriceChange function should revert if the prices are bad:

if (_oldPrice <= 0 || _newPrice <= 0) {
    // @audit should this revert instead?, keeper can handle it
    emit PriceChangeError(_oldPrice, _newPrice);
}

Impact

The keeper will report a valid upkeep when indeed it was invalid.

Recommended Mitigation Steps

Revert instead on price errors.

kumar-ish commented 3 years ago

Sorry for all the label changes; was gonna call a decision myself but probably good to open it to others 😅

kumar-ish commented 3 years ago

Sorry, will comment on this within half an hour

GalloDaSballo commented 3 years ago

The warden is saying that since there's a try catch here: https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/PoolKeeper.sol#L108

executePriceChange should revert on failure instead of emitting the error event: https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/LeveragedPool.sol#L179

The change could be implemented by refactoring the if: https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/LeveragedPool.sol#L178 into a require

I think the finding is correct, as well as the severity, this is in line with code arenas definition:

1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.