search

groLabs / GSquared

GNU General Public License v3.0
1 stars 0 forks source link

Weak oracle result staleness check in `staleCheck()` #18

Open kitty-the-kat opened 1 year ago

kitty-the-kat commented 1 year ago

staleCheck() checks if the Chainlink oracle price data is stale. The staleness check only checks if the timestamp is from the last 24 hours, but a stricter check would also check if the roundId is stale.

Technical Details

staleCheck() only checks that the Chainlink price data is under 24 hours old. The staleness check does not consider whether the roundId data may be outdated. It is recommended to do both, as shown in other security report findings here and here. Specifically, the DAI/USD oracle updates more regularly than every 24 hours. Considering that the Gro protocol has protections in place for stablecoins losing their peg, improving the Chainlink price staleness check is a crucial consideration.

Impact

Low. The staleness check could provide a false positive, say in the case that the price data is 23 hours old but is not from the most recent roundId.

Recommendation

Consider modifying setStrategies() to the following:

    function staleCheck(uint256 _updatedAt, uint80 _roundId, uint80 _answeredInRound) internal view returns (bool) {
        return ((block.timestamp - _updatedAt >= STALE_CHECK) && (_answeredInRound == _roundId));
    }
kitty-the-kat commented 1 year ago

Unclear what this is referring too - only chain link integration exist in the router oracle, and is currently not used by any contract if Im remember correctly - also not sure which setStrategies function is being referred to? Can you link line numbers

engn33r commented 1 year ago

@kitty-the-kat correct, staleCheck() is only called with the following call flows and the external functions usdToStable() and stableToUsd() in src/oracles/RouterOracle.sol are not used by the protocol.

  1. usdToStable() -> getPriceFeed() -> staleCheck()
  2. stableToUsd() -> getPriceFeed() -> staleCheck()

This is where staleCheck() is located: https://github.com/groLabs/GSquared-foundry/blob/f1831bdb353d4a0b3a8937087e1663f73b75e905/src/oracles/RouterOracle.sol#L121

kitty-the-kat commented 1 year ago

acknowledged - wont fix as chainlink oracle isn't used in this verison