`PoolKeeper.sol#performUpkeepSinglePool()` Wrong implementation allows attacker to interfere the upkeep of pools

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/PoolKeeper.sol#L90-L118

function performUpkeepSinglePool(address _pool) public override {
    uint256 startGas = gasleft();

    // validate the pool, check that the interval time has passed
    if (!checkUpkeepSinglePool(_pool)) {
        return;
    }
    ILeveragedPool pool = ILeveragedPool(_pool);
    (int256 latestPrice, bytes memory data, uint256 savedPreviousUpdatedTimestamp, uint256 updateInterval) = pool
        .getUpkeepInformation();

    // Start a new round
    // Get price in WAD format
    int256 lastExecutionPrice = executionPrice[_pool];
    executionPrice[_pool] = latestPrice;

    // This allows us to still batch multiple calls to executePriceChange, even if some are invalid
    // Without reverting the entire transaction
    try pool.poolUpkeep(lastExecutionPrice, latestPrice) {
        // If poolUpkeep is successful, refund the keeper for their gas costs
        uint256 gasSpent = startGas - gasleft();

        payKeeper(_pool, gasPrice, gasSpent, savedPreviousUpdatedTimestamp, updateInterval);
        emit UpkeepSuccessful(_pool, data, lastExecutionPrice, latestPrice);
    } catch Error(string memory reason) {
        // If poolUpkeep fails for any other reason, emit event
        emit PoolUpkeepError(_pool, reason);
    }
}

In the current implementation, when pool.poolUpkeep() fails (possibly because "Update interval hasn't passed"), the price change will not impact the long & short balances.

However, at L104 executionPrice[_pool] was updated to latestPrice already, so that it will be used as oldPrice in upcoming updates.

This means the current price change in this period is ignored, but the storage gets updated anyway, making the protocol believe it's the price change is been settled properly.

As a result, when the price goes up, the attacker can take advantage of this, and make the long positions have not benefited and the short positions have not suffered, vice versa.

Proof of Concept

Given:

• executionPrice[_pool] is now 100;
• The latestPrice rise to 200 within the updateInterval;

The attacker can:

1. Call performUpkeepSinglePool(): executionPrice[_pool] will get updated to 200, while shortBalance and longBalance of the LeveragedPool remain unchanged;
2. When the token price tank to 100 after intervalPassed, the attacker can call performUpkeepSinglePool() again:

The protocol now believes that the price of the token has fallen relative to the last updated price, therefore it will increase the balance of the short positions, and decrease the balance of the long positions.

Recommendation

Change to:

function performUpkeepSinglePool(address _pool) public override {
    uint256 startGas = gasleft();

    // validate the pool, check that the interval time has passed
    if (!checkUpkeepSinglePool(_pool)) {
        return;
    }
    ILeveragedPool pool = ILeveragedPool(_pool);
    (int256 latestPrice, bytes memory data, uint256 savedPreviousUpdatedTimestamp, uint256 updateInterval) = pool
        .getUpkeepInformation();

    // Start a new round
    // Get price in WAD format
    int256 lastExecutionPrice = executionPrice[_pool];

    // This allows us to still batch multiple calls to executePriceChange, even if some are invalid
    // Without reverting the entire transaction
    try pool.poolUpkeep(lastExecutionPrice, latestPrice) {
        // update lastExecutionPrice
        executionPrice[_pool] = latestPrice;

        // If poolUpkeep is successful, refund the keeper for their gas costs
        uint256 gasSpent = startGas - gasleft();

        payKeeper(_pool, gasPrice, gasSpent, savedPreviousUpdatedTimestamp, updateInterval);
        emit UpkeepSuccessful(_pool, data, lastExecutionPrice, latestPrice);
    } catch Error(string memory reason) {
        // If poolUpkeep fails for any other reason, emit event
        emit PoolUpkeepError(_pool, reason);
    }
}

[kumar-ish]

For this to happen, poolUpkeep would have to revert; the only way for this to happen (as pointed out by the warden) is if the update interval hasn't passed, OR for there to be an underlying bug in the contracts. However, as per the checkUpkeepSinglePool call on line 94, the function would never get to the try-catch if the update interval hasn't passed; thus, the only want for this to happen is there to be an underlying bug in the contracts that makes it revert, as we don't explicitly or implicitly allow any other way for it to. If the warden can prove there to be a way for poolUpkeep to revert (not withstanding the update interval not passing as that's already checked for), we would consider this to be a valid issue but otherwise we disagree with the severity as it requires there to be/needs to be paired wtith another bug in the contracts.

We will act on the suggestion nonetheless thought -- in case there's a regression and uncommit does have the ability to revert, thank you for that.

[GalloDaSballo]

I agree with the sponsor in that "in the case of a revert" doesn't really sound convincing for a high severity finding.

I appreciate the finding in that it does seem like it would be best to update the storage value only on success

I will downgrade to Low Severity, and think about this one over the weekend as there may be more