diff --git a/src/ReservoirPriceOracle.sol b/src/ReservoirPriceOracle.sol index 3e45213..acb13da 100644 --- a/src/ReservoirPriceOracle.sol +++ b/src/ReservoirPriceOracle.sol @@ -20,6 +20,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar using FixedPointMathLib for uint256; using LibSort for address[]; using RoutesLib for bytes32; + using Utils for uint256; using QueryProcessor for ReservoirPair; /////////////////////////////////////////////////////////////////////////////////////////////// @@ -107,8 +108,13 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar /// @param aToken1 Address of the higher token. /// @return rPrice The cached price of aToken0/aToken1 for simple routes. Returns 0 for prices of composite routes. /// @return rDecimalDiff The difference in decimals as defined by aToken1.decimals() - aToken0.decimals(). Only valid for simple routes. - function priceCache(address aToken0, address aToken1) external view returns (uint256 rPrice, int256 rDecimalDiff) { - (rPrice, rDecimalDiff) = _priceCache(aToken0, aToken1); + /// @return rBpDiffForMaxReward The number of basis points at and beyond which the bounty payout for a price update is maximum. + function priceCache(address aToken0, address aToken1) + external + view + returns (uint256 rPrice, int256 rDecimalDiff, uint256 rBpDiffForMaxReward) + { + (rPrice, rDecimalDiff, rBpDiffForMaxReward) = _priceCache(aToken0, aToken1); } /// @notice Updates the TWAP price for all simple routes between `aTokenA` and `aTokenB`. Will also update intermediate routes if the route defined between @@ -141,7 +147,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // if it's a simple route, we avoid loading the price again from storage if (lRoute.length != 2) { - (lPrevPrice,) = _priceCache(lToken0, lToken1); + (lPrevPrice,,) = _priceCache(lToken0, lToken1); } _writePriceCache(lToken0, lToken1, lNewPrice); @@ -169,29 +175,18 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar rResult = lPair.getTimeWeightedAverage(aQuery.priceType, aQuery.secs, aQuery.ago, lIndex); } - function _calcPercentageDiff(uint256 aOriginal, uint256 aNew) private pure returns (uint256) { - unchecked { - if (aOriginal == 0) return 0; - - // multiplication does not overflow as `aOriginal` and `aNew` is always less than or - // equal to `Constants.MAX_SUPPORTED_PRICE`, as checked in `_writePriceCache` - if (aOriginal > aNew) { - return (aOriginal - aNew) * 1e18 / aOriginal; - } else { - return (aNew - aOriginal) * 1e18 / aOriginal; - } - } - } - function _rewardUpdater(uint256 aPrevPrice, uint256 aNewPrice, address aRecipient, uint256 aBpDiffForMaxReward) private { - uint256 lPercentDiff = _calcPercentageDiff(aPrevPrice, aNewPrice); + uint256 lPercentDiff = aPrevPrice.calcPercentageDiff(aNewPrice); if (lPercentDiff == 0) return; if (aRecipient == address(0)) return; - // can be unchecked - uint256 lBpForMaxRewardWAD = aBpDiffForMaxReward * Constants.WAD / Constants.BP_SCALE; + // SAFETY: this mul will not overflow as `aBpDiffForMaxReward` is capped by `Constants.BP_SCALE` + uint256 lBpForMaxRewardWAD; + unchecked { + lBpForMaxRewardWAD = aBpDiffForMaxReward * Constants.WAD / Constants.BP_SCALE; + } uint256 lReward = lPercentDiff > lBpForMaxRewardWAD ? lBpForMaxRewardWAD : lPercentDiff; // N.B. Revisit this whenever deployment on a new chain is needed @@ -287,8 +282,12 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar } } - // performs an SLOAD to load 1 word which contains the simple price and decimal difference - function _priceCache(address aToken0, address aToken1) private view returns (uint256 rPrice, int256 rDecimalDiff) { + // performs an SLOAD to load 1 word which contains the simple price, decimal difference, and number of basis points for maximum reward + function _priceCache(address aToken0, address aToken1) + private + view + returns (uint256 rPrice, int256 rDecimalDiff, uint256 rBpDiffForMaxReward) + { bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1); bytes32 lData; @@ -298,6 +297,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar if (lData.isSimplePrice()) { rPrice = lData.getPrice(); rDecimalDiff = lData.getDecimalDifference(); + rBpDiffForMaxReward = lData.getBpDiffForMaxReward(); } } @@ -357,7 +357,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar for (uint256 i = 0; i < lRoute.length - 1; ++i) { (lToken0, lToken1) = Utils.sortTokens(lRoute[i], lRoute[i + 1]); // it is assumed that intermediate routes defined here are simple routes and not composite routes - (lPrice, lDecimalDiff) = _priceCache(lToken0, lToken1); + (lPrice, lDecimalDiff,) = _priceCache(lToken0, lToken1); if (lPrice == 0) revert OracleErrors.PriceZero(); lIntermediateAmount = _calcAmtOut(lIntermediateAmount, lPrice, lDecimalDiff, lRoute[i] != lToken0); diff --git a/src/libraries/Utils.sol b/src/libraries/Utils.sol index 2e8dae8..f7ec76a 100644 --- a/src/libraries/Utils.sol +++ b/src/libraries/Utils.sol @@ -11,4 +11,18 @@ library Utils { function calculateSlot(address aToken0, address aToken1) internal pure returns (bytes32) { return keccak256(abi.encode(aToken0, aToken1)); } + + /// @dev Assumes that `aOriginal` and `aNew` is less than or equal to + /// `Constants.MAX_SUPPORTED_PRICE`. So multiplication by 1e18 will not overflow. + function calcPercentageDiff(uint256 aOriginal, uint256 aNew) internal pure returns (uint256) { + unchecked { + if (aOriginal == 0) return 0; + + if (aOriginal > aNew) { + return (aOriginal - aNew) * 1e18 / aOriginal; + } else { + return (aNew - aOriginal) * 1e18 / aOriginal; + } + } + } } diff --git a/test/unit/ReservoirPriceOracle.t.sol b/test/unit/ReservoirPriceOracle.t.sol index 9ded8d1..c8017e2 100644 --- a/test/unit/ReservoirPriceOracle.t.sol +++ b/test/unit/ReservoirPriceOracle.t.sol @@ -44,7 +44,7 @@ contract ReservoirPriceOracleTest is BaseTest { // writes the cached prices, for easy testing function _writePriceCache(address aToken0, address aToken1, uint256 aPrice) internal { require(aToken0 < aToken1, "tokens unsorted"); - require(bytes32(aPrice) & bytes2(0xffff) == 0, "PRICE WILL OVERLAP FLAG"); + require(aPrice <= Constants.MAX_SUPPORTED_PRICE, "price too large"); vm.record(); _oracle.priceCache(aToken0, aToken1); @@ -98,7 +98,7 @@ contract ReservoirPriceOracleTest is BaseTest { _writePriceCache(address(_tokenB), address(_tokenC), lPrice); // assert - (uint256 lQueriedPrice,) = _oracle.priceCache(address(_tokenB), address(_tokenC)); + (uint256 lQueriedPrice,,) = _oracle.priceCache(address(_tokenB), address(_tokenC)); assertEq(lQueriedPrice, lPrice); } @@ -124,7 +124,7 @@ contract ReservoirPriceOracleTest is BaseTest { // arrange _writePriceCache(address(_tokenA), address(_tokenB), lPrice); - (uint256 lQueriedPrice,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + (uint256 lQueriedPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lQueriedPrice, lPrice); // act @@ -219,11 +219,11 @@ contract ReservoirPriceOracleTest is BaseTest { lRoute[2] = address(lTokenC); { - uint16[] memory lBpDiffForMaxReward = new uint16[](2); - lBpDiffForMaxReward[0] = lBpDiffForMaxReward[1] = Constants.BP_SCALE; - _oracle.setRoute(address(lTokenA), address(lTokenC), lRoute, lBpDiffForMaxReward); - _writePriceCache(address(lTokenA), address(lTokenB), 1e18); - _writePriceCache(address(lTokenC), address(lTokenB), 1e18); + uint16[] memory lBpDiffForMaxReward = new uint16[](2); + lBpDiffForMaxReward[0] = lBpDiffForMaxReward[1] = Constants.BP_SCALE; + _oracle.setRoute(address(lTokenA), address(lTokenC), lRoute, lBpDiffForMaxReward); + _writePriceCache(address(lTokenA), address(lTokenB), 1e18); + _writePriceCache(address(lTokenC), address(lTokenB), 1e18); } // act @@ -415,6 +415,17 @@ contract ReservoirPriceOracleTest is BaseTest { assertEq(lAmtOut / 1e12, lAmtIn * lRate / 1e18); } + function testPriceCache_Inverted() external { + // arrange + _writePriceCache(address(_tokenA), address(_tokenB), 1e18); + + // act + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); + + // assert + assertEq(lPrice, 0); + } + function testUpdateTwapPeriod(uint256 aNewPeriod) external { // assume uint64 lNewPeriod = uint64(bound(aNewPeriod, 1, 1 hours)); @@ -439,9 +450,9 @@ contract ReservoirPriceOracleTest is BaseTest { assertEq(_oracle.rewardGasAmount(), lNewRewardMultiplier); } - function testUpdatePrice_FirstUpdate() external { + function testUpdatePrice_FirstUpdate() public { // sanity - (uint256 lPrice,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lPrice, 0); // arrange @@ -450,23 +461,41 @@ contract ReservoirPriceOracleTest is BaseTest { skip(1); _pair.sync(); skip(_oracle.twapPeriod() * 2); - _tokenA.mint(address(_pair), 2e18); - _pair.swap(2e18, true, address(this), ""); + _pair.sync(); // act _oracle.updatePrice(address(_tokenB), address(_tokenA), address(this)); // assert - (lPrice,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + (lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lPrice, 98_918_868_099_219_913_512); - (lPrice,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); + (lPrice,,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); assertEq(lPrice, 0); assertEq(address(this).balance, 0); // there should be no reward for the first price update } - function testUpdatePrice_WithinThreshold() external { + function testUpdatePrice_NoPriceChange() external { // arrange - _writePriceCache(address(_tokenA), address(_tokenB), 98.9223e18); + testUpdatePrice_FirstUpdate(); + uint256 lExpectedPrice = 98_918_868_099_219_913_512; + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + assertEq(lPrice, lExpectedPrice); // ensure that there is a price to begin with + skip(_oracle.twapPeriod() * 2); + _pair.sync(); + + // act + _oracle.updatePrice(address(_tokenA), address(_tokenB), address(this)); + + // assert + (lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + assertEq(lPrice, lExpectedPrice); + assertEq(address(this).balance, 0); // no reward as the price did not change at all + } + + function testUpdatePrice_BelowMaxReward() external { + // arrange + uint256 lOriginalPrice = 98.9223e18; + _writePriceCache(address(_tokenA), address(_tokenB), lOriginalPrice); deal(address(_oracle), 1 ether); skip(1); @@ -479,14 +508,14 @@ contract ReservoirPriceOracleTest is BaseTest { _oracle.updatePrice(address(_tokenB), address(_tokenA), address(this)); // assert - (uint256 lPrice,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lPrice, 98_918_868_099_219_913_512); - (lPrice,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); - assertEq(lPrice, 0); - assertEq(address(this).balance, 0); // no reward since price is within threshold + uint256 lExpectedRewardReceived = + block.basefee * _oracle.rewardGasAmount() * lOriginalPrice.calcPercentageDiff(lPrice) / 1e18; + assertEq(address(this).balance, lExpectedRewardReceived); // some reward received but is less than max possible reward } - function testUpdatePrice_BeyondThreshold() external { + function testUpdatePrice_BeyondMaxReward() external { // arrange _writePriceCache(address(_tokenA), address(_tokenB), 5e18); deal(address(_oracle), 1 ether); @@ -501,15 +530,16 @@ contract ReservoirPriceOracleTest is BaseTest { _oracle.updatePrice(address(_tokenB), address(_tokenA), address(this)); // assert - (uint256 lPrice,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lPrice, 98_918_868_099_219_913_512); - (lPrice,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); + (lPrice,,) = _oracle.priceCache(address(_tokenB), address(_tokenA)); assertEq(lPrice, 0); - assertEq(address(this).balance, block.basefee * _oracle.rewardGasAmount()); - assertEq(address(_oracle).balance, 1 ether - block.basefee * _oracle.rewardGasAmount()); + uint256 lExpectedRewardReceived = block.basefee * _oracle.rewardGasAmount(); + assertEq(address(this).balance, lExpectedRewardReceived); + assertEq(address(_oracle).balance, 1 ether - lExpectedRewardReceived); } - function testUpdatePrice_BeyondThreshold_InsufficientReward(uint256 aRewardAvailable) external { + function testUpdatePrice_RewardEligible_InsufficientReward(uint256 aRewardAvailable) external { // assume uint256 lRewardAvailable = bound(aRewardAvailable, 1, block.basefee * _oracle.rewardGasAmount() - 1); @@ -526,11 +556,13 @@ contract ReservoirPriceOracleTest is BaseTest { // act _oracle.updatePrice(address(_tokenA), address(_tokenB), address(this)); - // assert - assertEq(address(this).balance, 0); // no reward as there's insufficient ether in the contract + // assert - no reward as there's insufficient ether in the contract, but price cache updated nonetheless + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + assertNotEq(lPrice, 5e18); + assertEq(address(this).balance, 0); } - function testUpdatePrice_BeyondThreshold_ZeroRecipient() external { + function testUpdatePrice_RewardEligible_ZeroRecipient() external { // arrange uint256 lBalance = 10 ether; deal(address(_oracle), lBalance); @@ -545,7 +577,9 @@ contract ReservoirPriceOracleTest is BaseTest { // act _oracle.updatePrice(address(_tokenA), address(_tokenB), address(0)); - // assert - no change to balance + // assert - no change to balance, but price cache updated nonetheless + (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); + assertNotEq(lPrice, 5e18); assertEq(address(_oracle).balance, lBalance); } @@ -595,10 +629,10 @@ contract ReservoirPriceOracleTest is BaseTest { _oracle.updatePrice(address(_tokenA), address(_tokenB), address(this)); // assert - (uint256 lPriceAC,) = _oracle.priceCache(lStart, lIntermediate1); - (uint256 lPriceCD,) = _oracle.priceCache(lIntermediate1, lIntermediate2); - (uint256 lPriceBD,) = _oracle.priceCache(lEnd, lIntermediate2); - (uint256 lPriceAB,) = _oracle.priceCache(lStart, lEnd); + (uint256 lPriceAC,,) = _oracle.priceCache(lStart, lIntermediate1); + (uint256 lPriceCD,,) = _oracle.priceCache(lIntermediate1, lIntermediate2); + (uint256 lPriceBD,,) = _oracle.priceCache(lEnd, lIntermediate2); + (uint256 lPriceAB,,) = _oracle.priceCache(lStart, lEnd); assertApproxEqRel(lPriceAC, 0.5e18, 0.0001e18); assertApproxEqRel(lPriceCD, 2e18, 0.0001e18); assertApproxEqRel(lPriceBD, 2e18, 0.0001e18); @@ -623,7 +657,7 @@ contract ReservoirPriceOracleTest is BaseTest { // assert address[] memory lQueriedRoute = _oracle.route(lToken0, lToken1); assertEq(lQueriedRoute, lRoute); - (, int256 lDecimalDiff) = _oracle.priceCache(lToken0, lToken1); + (, int256 lDecimalDiff,) = _oracle.priceCache(lToken0, lToken1); int256 lActualDiff = int256(uint256(IERC20(lToken1).decimals())) - int256(uint256(IERC20(lToken0).decimals())); assertEq(lDecimalDiff, lActualDiff); } @@ -720,7 +754,7 @@ contract ReservoirPriceOracleTest is BaseTest { // assert lQueriedRoute = _oracle.route(lToken0, lToken1); assertEq(lQueriedRoute, new address[](0)); - (uint256 lPrice,) = _oracle.priceCache(lToken0, lToken1); + (uint256 lPrice,,) = _oracle.priceCache(lToken0, lToken1); assertEq(lPrice, 0); }