From c8df956436eefbc17ee970c65fdeb0f668dc7dbd Mon Sep 17 00:00:00 2001 From: "A.L." Date: Mon, 16 Dec 2024 19:18:28 +0800 Subject: [PATCH] fix: cantina audit findings (#25) * fix: addresses #2 and #3 * fix: addresses #4 * fix: addresses #8 * fix: addresses #1 * fix: addresses #9 * fix: addresses #7 * fix: addresses #5 * fix: addresses #6 * test: impl better workaround to ensure fuzz does not give address with existing code * test: more assumes * gas: update snapshot * test: assumeNotForgeAddress * gas: update snapshot * fix: impl fix for attempting to use a composite route as an intermediate route * test: add case for clearing out second slot when replacing by a shorter route * gas: update snapshot * test: add assumptions that addresses do not clash * gas: update snapshot --- .gas-snapshot | 145 +++++++------ src/ReservoirPriceOracle.sol | 81 ++++--- src/interfaces/IPriceOracle.sol | 31 +-- src/libraries/Constants.sol | 4 +- src/libraries/OracleErrors.sol | 2 +- src/libraries/RoutesLib.sol | 12 +- src/libraries/Utils.sol | 1 + test/__fixtures/BaseTest.t.sol | 2 +- test/large/ReservoirPriceOracleLarge.t.sol | 24 ++- test/unit/ReservoirPriceOracle.t.sol | 236 +++++++++++++++------ test/unit/libraries/RoutesLib.t.sol | 8 +- test/unit/libraries/Utils.t.sol | 44 ++++ 12 files changed, 393 insertions(+), 197 deletions(-) create mode 100644 test/unit/libraries/Utils.t.sol diff --git a/.gas-snapshot b/.gas-snapshot index 712c3c7..c1ca280 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,79 +1,86 @@ -QueryProcessorTest:testFindNearestSample_CanFindExactValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 69293044, ~: 77954902) -QueryProcessorTest:testFindNearestSample_CanFindIntermediateValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 68012389, ~: 77774179) +QueryProcessorTest:testFindNearestSample_CanFindExactValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 69006651, ~: 78950566) +QueryProcessorTest:testFindNearestSample_CanFindIntermediateValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 67181426, ~: 77952295) QueryProcessorTest:testFindNearestSample_NotInitialized() (gas: 1056944146) -QueryProcessorTest:testFindNearestSample_OneSample(uint256) (runs: 256, μ: 80623, ~: 80652) +QueryProcessorTest:testFindNearestSample_OneSample(uint256) (runs: 256, μ: 80622, ~: 80652) QueryProcessorTest:testGetInstantValue() (gas: 124418) QueryProcessorTest:testGetInstantValue_NotInitialized(uint256) (runs: 256, μ: 19400, ~: 19400) -QueryProcessorTest:testGetInstantValue_NotInitialized_BeyondBufferSize(uint8,uint16) (runs: 256, μ: 69204583, ~: 69204518) -QueryProcessorTest:testGetPastAccumulator_BufferEmpty(uint8) (runs: 256, μ: 26911, ~: 26984) -QueryProcessorTest:testGetPastAccumulator_ExactMatch(uint32,uint256,uint256,uint16) (runs: 256, μ: 72113529, ~: 81319251) -QueryProcessorTest:testGetPastAccumulator_ExactMatch_LatestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 69463960, ~: 78425849) -QueryProcessorTest:testGetPastAccumulator_ExactMatch_OldestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 69493894, ~: 78457616) -QueryProcessorTest:testGetPastAccumulator_ExtrapolatesBeyondLatest(uint32,uint256,uint256,uint256) (runs: 256, μ: 67986036, ~: 77745180) -QueryProcessorTest:testGetPastAccumulator_InterpolatesBetweenPastAccumulators(uint32,uint256,uint256,uint256) (runs: 256, μ: 68020346, ~: 77780471) -QueryProcessorTest:testGetPastAccumulator_InvalidAgo(uint32,uint256,uint256,uint256) (runs: 256, μ: 67977605, ~: 77737553) -QueryProcessorTest:testGetPastAccumulator_QueryTooOld(uint32,uint256,uint256,uint256) (runs: 256, μ: 67989141, ~: 77747128) -QueryProcessorTest:testGetTimeWeightedAverage(uint32,uint256,uint256,uint256,uint256) (runs: 256, μ: 103407330, ~: 109858182) +QueryProcessorTest:testGetInstantValue_NotInitialized_BeyondBufferSize(uint8,uint16) (runs: 256, μ: 69204610, ~: 69204518) +QueryProcessorTest:testGetPastAccumulator_BufferEmpty(uint8) (runs: 256, μ: 26921, ~: 26984) +QueryProcessorTest:testGetPastAccumulator_ExactMatch(uint32,uint256,uint256,uint16) (runs: 256, μ: 71299667, ~: 79601317) +QueryProcessorTest:testGetPastAccumulator_ExactMatch_LatestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 69601383, ~: 77006769) +QueryProcessorTest:testGetPastAccumulator_ExactMatch_OldestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 69631465, ~: 77038536) +QueryProcessorTest:testGetPastAccumulator_ExtrapolatesBeyondLatest(uint32,uint256,uint256,uint256) (runs: 256, μ: 67155174, ~: 77923691) +QueryProcessorTest:testGetPastAccumulator_InterpolatesBetweenPastAccumulators(uint32,uint256,uint256,uint256) (runs: 256, μ: 67189366, ~: 77958587) +QueryProcessorTest:testGetPastAccumulator_InvalidAgo(uint32,uint256,uint256,uint256) (runs: 256, μ: 67146760, ~: 77915383) +QueryProcessorTest:testGetPastAccumulator_QueryTooOld(uint32,uint256,uint256,uint256) (runs: 256, μ: 67158275, ~: 77925299) +QueryProcessorTest:testGetTimeWeightedAverage(uint32,uint256,uint256,uint256,uint256) (runs: 256, μ: 100879462, ~: 107747925) QueryProcessorTest:testGetTimeWeightedAverage_BadSecs() (gas: 10981) -ReservoirPriceOracleTest:testClearRoute() (gas: 52209) -ReservoirPriceOracleTest:testClearRoute_AllWordsCleared() (gas: 159642) -ReservoirPriceOracleTest:testDesignatePair() (gas: 29089) -ReservoirPriceOracleTest:testDesignatePair_IncorrectPair() (gas: 21129) -ReservoirPriceOracleTest:testDesignatePair_NotOwner() (gas: 17515) -ReservoirPriceOracleTest:testDesignatePair_TokenOrderReversed() (gas: 30670) -ReservoirPriceOracleTest:testGetQuote(uint256,uint256) (runs: 256, μ: 33680, ~: 33783) -ReservoirPriceOracleTest:testGetQuote_AmountInTooLarge() (gas: 13019) -ReservoirPriceOracleTest:testGetQuote_BaseIsVault(uint256) (runs: 256, μ: 401362, ~: 401124) -ReservoirPriceOracleTest:testGetQuote_ComplicatedDecimals() (gas: 10361941) -ReservoirPriceOracleTest:testGetQuote_ERC4626AssetFails() (gas: 21434) -ReservoirPriceOracleTest:testGetQuote_Inverse(uint256,uint256) (runs: 256, μ: 35858, ~: 36022) -ReservoirPriceOracleTest:testGetQuote_MultipleHops() (gas: 111597) -ReservoirPriceOracleTest:testGetQuote_MultipleHops_Inverse() (gas: 111891) -ReservoirPriceOracleTest:testGetQuote_MultipleHops_PriceZero() (gas: 122213) -ReservoirPriceOracleTest:testGetQuote_NoFallbackOracle() (gas: 20830) -ReservoirPriceOracleTest:testGetQuote_PriceZero() (gas: 15942) -ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_1HopRoute(uint256,uint256,address,address,uint8,uint8) (runs: 256, μ: 5316105, ~: 5316096) -ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_2HopRoute(uint256,uint256,uint256,address,address,address,uint8,uint8,uint8) (runs: 256, μ: 10503666, ~: 10503746) -ReservoirPriceOracleTest:testGetQuote_SameBaseQuote(uint256,address) (runs: 256, μ: 8972, ~: 8972) -ReservoirPriceOracleTest:testGetQuote_UseFallback() (gas: 38232) -ReservoirPriceOracleTest:testGetQuote_ZeroIn() (gas: 36638) -ReservoirPriceOracleTest:testGetQuotes(uint256,uint256) (runs: 256, μ: 26236, ~: 26339) -ReservoirPriceOracleTest:testName() (gas: 9355) -ReservoirPriceOracleTest:testPriceCache_Inverted() (gas: 22028) -ReservoirPriceOracleTest:testSetFallbackOracle_NotOwner() (gas: 10993) -ReservoirPriceOracleTest:testSetRoute() (gas: 60992) -ReservoirPriceOracleTest:testSetRoute_InvalidDecimals() (gas: 761313) -ReservoirPriceOracleTest:testSetRoute_InvalidRewardThreshold() (gas: 37234) -ReservoirPriceOracleTest:testSetRoute_InvalidRewardThresholdLength() (gas: 18050) -ReservoirPriceOracleTest:testSetRoute_InvalidRoute() (gas: 20151) -ReservoirPriceOracleTest:testSetRoute_InvalidRouteLength() (gas: 19209) -ReservoirPriceOracleTest:testSetRoute_MultipleHops() (gas: 201150) -ReservoirPriceOracleTest:testSetRoute_NotSorted() (gas: 13029) -ReservoirPriceOracleTest:testSetRoute_OverwriteExisting() (gas: 169526) -ReservoirPriceOracleTest:testSetRoute_SameToken() (gas: 12975) -ReservoirPriceOracleTest:testUndesignatePair() (gas: 30263) +ReservoirPriceOracleTest:testClearRoute() (gas: 52358) +ReservoirPriceOracleTest:testClearRoute_AllWordsCleared() (gas: 160260) +ReservoirPriceOracleTest:testDesignatePair() (gas: 29133) +ReservoirPriceOracleTest:testDesignatePair_IncorrectPair() (gas: 21174) +ReservoirPriceOracleTest:testDesignatePair_NotOwner() (gas: 17537) +ReservoirPriceOracleTest:testDesignatePair_TokenOrderReversed() (gas: 30736) +ReservoirPriceOracleTest:testGetQuote(uint256,uint256) (runs: 256, μ: 33774, ~: 33869) +ReservoirPriceOracleTest:testGetQuote_AmountInTooLarge() (gas: 12952) +ReservoirPriceOracleTest:testGetQuote_BaseIsVault(uint256) (runs: 256, μ: 401427, ~: 401176) +ReservoirPriceOracleTest:testGetQuote_ComplicatedDecimals() (gas: 10362427) +ReservoirPriceOracleTest:testGetQuote_ERC4626AssetFails() (gas: 21389) +ReservoirPriceOracleTest:testGetQuote_Inverse(uint256,uint256) (runs: 256, μ: 35952, ~: 36108) +ReservoirPriceOracleTest:testGetQuote_MultipleHops() (gas: 112160) +ReservoirPriceOracleTest:testGetQuote_MultipleHops_Inverse() (gas: 112386) +ReservoirPriceOracleTest:testGetQuote_MultipleHops_PriceZero() (gas: 122794) +ReservoirPriceOracleTest:testGetQuote_NoFallbackOracle() (gas: 20785) +ReservoirPriceOracleTest:testGetQuote_PriceZero() (gas: 15919) +ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_1HopRoute(uint256,uint256,address,address,uint8,uint8) (runs: 256, μ: 5229872, ~: 5229880) +ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_2HopRoute(uint256,uint256,uint256,address,address,address,uint8,uint8,uint8) (runs: 256, μ: 10377471, ~: 10377571) +ReservoirPriceOracleTest:testGetQuote_SameBaseQuote(uint256,address) (runs: 256, μ: 8971, ~: 8971) +ReservoirPriceOracleTest:testGetQuote_UseFallback() (gas: 38209) +ReservoirPriceOracleTest:testGetQuote_ZeroIn() (gas: 36679) +ReservoirPriceOracleTest:testGetQuotes(uint256,uint256) (runs: 256, μ: 26353, ~: 26448) +ReservoirPriceOracleTest:testName() (gas: 9311) +ReservoirPriceOracleTest:testPriceCache_Inverted() (gas: 22091) +ReservoirPriceOracleTest:testSetFallbackOracle_NotOwner() (gas: 10928) +ReservoirPriceOracleTest:testSetRoute() (gas: 61115) +ReservoirPriceOracleTest:testSetRoute_2ndSlotClearedWhenReplacing3HopByShorterRoute() (gas: 167426) +ReservoirPriceOracleTest:testSetRoute_InvalidDecimals() (gas: 763534) +ReservoirPriceOracleTest:testSetRoute_InvalidRewardThreshold() (gas: 41610) +ReservoirPriceOracleTest:testSetRoute_InvalidRewardThresholdLength() (gas: 18129) +ReservoirPriceOracleTest:testSetRoute_InvalidRoute() (gas: 20221) +ReservoirPriceOracleTest:testSetRoute_InvalidRouteLength() (gas: 19356) +ReservoirPriceOracleTest:testSetRoute_MultipleHops() (gas: 201780) +ReservoirPriceOracleTest:testSetRoute_NotSorted() (gas: 13074) +ReservoirPriceOracleTest:testSetRoute_OverwriteExisting() (gas: 173710) +ReservoirPriceOracleTest:testSetRoute_OverwriteExisting_UsingCompositeAsIntermediate() (gas: 114782) +ReservoirPriceOracleTest:testSetRoute_ReplaceExistingCompositeWithSimple() (gas: 182717) +ReservoirPriceOracleTest:testSetRoute_ReplaceExistingSimpleWithComposite() (gas: 103503) +ReservoirPriceOracleTest:testSetRoute_SameToken() (gas: 13020) +ReservoirPriceOracleTest:testUndesignatePair() (gas: 30285) ReservoirPriceOracleTest:testUndesignatePair_NotOwner() (gas: 15315) -ReservoirPriceOracleTest:testUpdatePrice_AboveThresholdBelowMaxReward(uint256) (runs: 256, μ: 164770, ~: 164790) -ReservoirPriceOracleTest:testUpdatePrice_BelowThreshold(uint256) (runs: 256, μ: 149645, ~: 149316) -ReservoirPriceOracleTest:testUpdatePrice_BeyondMaxReward(uint256) (runs: 256, μ: 162201, ~: 162226) -ReservoirPriceOracleTest:testUpdatePrice_FirstUpdate() (gas: 153384) -ReservoirPriceOracleTest:testUpdatePrice_IntermediateRoutes() (gas: 11080585) -ReservoirPriceOracleTest:testUpdatePrice_NoPath() (gas: 15986) -ReservoirPriceOracleTest:testUpdatePrice_PriceOutOfRange() (gas: 5374104) -ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_ContractNoReceive() (gas: 152500) -ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_InsufficientReward(uint256) (runs: 256, μ: 210828, ~: 211040) -ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_ZeroRecipient() (gas: 146179) -ReservoirPriceOracleTest:testUpdatePrice_WriteToNonSimpleRoute() (gas: 498183) -ReservoirPriceOracleTest:testUpdateRewardGasAmount() (gas: 19038) -ReservoirPriceOracleTest:testUpdateRewardGasAmount_NotOwner() (gas: 10953) -ReservoirPriceOracleTest:testUpdateTwapPeriod(uint256) (runs: 256, μ: 21613, ~: 21687) -ReservoirPriceOracleTest:testUpdateTwapPeriod_InvalidTwapPeriod(uint256) (runs: 256, μ: 17688, ~: 17994) -ReservoirPriceOracleTest:testValidatePair_NoDesignatedPair() (gas: 119204) -ReservoirPriceOracleTest:testWritePriceCache(uint256) (runs: 256, μ: 30001, ~: 29763) +ReservoirPriceOracleTest:testUpdatePrice_AboveThresholdBelowMaxReward(uint256) (runs: 256, μ: 162400, ~: 162426) +ReservoirPriceOracleTest:testUpdatePrice_BelowThreshold(uint256) (runs: 256, μ: 149819, ~: 149851) +ReservoirPriceOracleTest:testUpdatePrice_BeyondMaxReward(uint256) (runs: 256, μ: 159761, ~: 159835) +ReservoirPriceOracleTest:testUpdatePrice_FirstUpdate() (gas: 153341) +ReservoirPriceOracleTest:testUpdatePrice_IntermediateRoutes() (gas: 11082572) +ReservoirPriceOracleTest:testUpdatePrice_NoPath() (gas: 15942) +ReservoirPriceOracleTest:testUpdatePrice_PriceOutOfRange() (gas: 5374118) +ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_ContractNoReceive() (gas: 150542) +ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_InsufficientReward(uint256) (runs: 256, μ: 208872, ~: 209191) +ReservoirPriceOracleTest:testUpdatePrice_RewardEligible_ZeroRecipient() (gas: 144199) +ReservoirPriceOracleTest:testUpdatePrice_WriteToNonSimpleRoute() (gas: 502364) +ReservoirPriceOracleTest:testUpdateRewardGasAmount() (gas: 19117) +ReservoirPriceOracleTest:testUpdateRewardGasAmount_NotOwner() (gas: 10987) +ReservoirPriceOracleTest:testUpdateTwapPeriod(uint256) (runs: 256, μ: 21699, ~: 21778) +ReservoirPriceOracleTest:testUpdateTwapPeriod_InvalidTwapPeriod(uint256) (runs: 256, μ: 17869, ~: 18066) +ReservoirPriceOracleTest:testValidatePair_NoDesignatedPair() (gas: 119160) +ReservoirPriceOracleTest:testWritePriceCache(uint256) (runs: 256, μ: 30089, ~: 29838) RoutesLibTest:testGetDecimalDifference() (gas: 3966) RoutesLibTest:testIsCompositeRoute() (gas: 4332) -RoutesLibTest:testPackSimplePrice(int8,uint256) (runs: 256, μ: 8083, ~: 7862) +RoutesLibTest:testPackSimplePrice(int8,uint64,uint256) (runs: 256, μ: 8159, ~: 7915) SamplesTest:testAccumulator() (gas: 3930) SamplesTest:testAccumulator_BadVariableRequest() (gas: 3355) SamplesTest:testInstant() (gas: 3880) -SamplesTest:testInstant_BadVariableRequest() (gas: 3376) \ No newline at end of file +SamplesTest:testInstant_BadVariableRequest() (gas: 3376) +UtilsTest:testCalcPercentageDiff_Double(uint256) (runs: 256, μ: 7055, ~: 6836) +UtilsTest:testCalcPercentageDiff_Half(uint256) (runs: 256, μ: 7235, ~: 7432) +UtilsTest:testCalcPercentageDiff_NoDiff(uint256) (runs: 256, μ: 6838, ~: 6622) diff --git a/src/ReservoirPriceOracle.sol b/src/ReservoirPriceOracle.sol index 2289b1a..ab6a884 100644 --- a/src/ReservoirPriceOracle.sol +++ b/src/ReservoirPriceOracle.sol @@ -48,17 +48,17 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // STORAGE // /////////////////////////////////////////////////////////////////////////////////////////////// + // The following 3 storage variables take up 1 storage slot. + /// @notice The PriceOracle to call if this router is not configured for base/quote. /// @dev If `address(0)` then there is no fallback. address public fallbackOracle; - // The following 2 storage variables take up 1 storage slot. - /// @notice This number is multiplied by the base fee to determine the reward for keepers. uint64 public rewardGasAmount; /// @notice TWAP period (in seconds) for querying the oracle. - uint64 public twapPeriod; + uint16 public twapPeriod; /// @notice Designated pairs to serve as price feed for a certain token0 and token1. mapping(address token0 => mapping(address token1 => ReservoirPair pair)) public pairs; @@ -67,7 +67,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // CONSTRUCTOR, FALLBACKS // /////////////////////////////////////////////////////////////////////////////////////////////// - constructor(uint64 aTwapPeriod, uint64 aMultiplier, PriceType aType) { + constructor(uint16 aTwapPeriod, uint64 aMultiplier, PriceType aType) { updateTwapPeriod(aTwapPeriod); updateRewardGasAmount(aMultiplier); PRICE_TYPE = aType; @@ -83,6 +83,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // IPriceOracle + /// @inheritdoc IPriceOracle function name() external pure returns (string memory) { return "RESERVOIR PRICE ORACLE"; } @@ -103,6 +104,13 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // price update related functions + /// @notice Returns the defined route between a given pair of tokens if there is one. + /// @param aToken0 Address of the lower token. + /// @param aToken1 Address of the higher token. + /// @return rRoute An array containing the tokens that make up the route. + /// Contains two elements if it is a simple route (A->B), + /// Contains three elements if it is a 2 hop route (A->B->C) and so on. + /// Returns an empty array if there is no route, or if the arg tokens are not sorted. function route(address aToken0, address aToken1) external view returns (address[] memory rRoute) { (rRoute,,,) = _getRouteDecimalDifferencePrice(aToken0, aToken1); } @@ -115,7 +123,7 @@ 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. - /// @return rRewardThreshold The number of basis points of difference in price at and beyond which a reward is applicable for a price update. + /// @return rRewardThreshold The difference in price in WAD at and beyond which a reward is applicable for a price update. function priceCache(address aToken0, address aToken1) external view @@ -199,21 +207,15 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar view returns (uint256 rReward) { - // SAFETY: this mul will not overflow as 0 < `aRewardThreshold` <= `Constants.BP_SCALE`, as checked by `setRoute` - uint256 lRewardThresholdWAD; - unchecked { - lRewardThresholdWAD = aRewardThreshold * Constants.WAD / Constants.BP_SCALE; - } - uint256 lPercentDiff = aPrevPrice.calcPercentageDiff(aNewPrice); // SAFETY: this mul will not overflow even in extreme cases of `block.basefee`. unchecked { - if (lPercentDiff < lRewardThresholdWAD) { + if (lPercentDiff < aRewardThreshold) { return 0; } // payout max reward - else if (lPercentDiff >= lRewardThresholdWAD * MAX_REWARD_MULTIPLIER) { + else if (lPercentDiff >= aRewardThreshold * MAX_REWARD_MULTIPLIER) { // N.B. Revisit this whenever deployment on a new chain is needed // // we use `block.basefee` instead of `ArbGasInfo::getMinimumGasPrice()` @@ -222,10 +224,8 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // congestion rReward = block.basefee * rewardGasAmount * MAX_REWARD_MULTIPLIER; } else { - assert( - lPercentDiff >= lRewardThresholdWAD && lPercentDiff < lRewardThresholdWAD * MAX_REWARD_MULTIPLIER - ); - rReward = block.basefee * rewardGasAmount * lPercentDiff / lRewardThresholdWAD; // denominator is never 0 + // denominator is never 0 as checked by `setRoute` + rReward = block.basefee * rewardGasAmount * lPercentDiff / aRewardThreshold; } } } @@ -242,7 +242,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar /// @return rRoute The route to determine the price between aToken0 and aToken1. Returns an empty array if there is no route. /// @return rDecimalDiff The result of token1.decimals() - token0.decimals() if it's a simple route. 0 otherwise. /// @return rPrice The price of aToken0/aToken1 if it's a simple route (i.e. rRoute.length == 2). 0 otherwise. - /// @return rRewardThreshold The number of basis points of difference in price at and beyond which a reward is applicable for a price update. + /// @return rRewardThreshold The difference in price in WAD at and beyond which a reward is applicable for a price update. function _getRouteDecimalDifferencePrice(address aToken0, address aToken1) private view @@ -295,7 +295,8 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar // Calculate the storage slot for this intermediate segment and read it to see if there is an existing // route. If there isn't an existing route, we create one as well. - function _checkAndPopulateIntermediateRoute(address aTokenA, address aTokenB, uint16 aBpMaxReward) private { + // Will revert if it attempts to use an intermediate route that is not a simple route. + function _checkAndPopulateIntermediateRoute(address aTokenA, address aTokenB, uint64 aBpMaxReward) private { (address lToken0, address lToken1) = Utils.sortTokens(aTokenA, aTokenB); bytes32 lSlot = Utils.calculateSlot(lToken0, lToken1); @@ -307,9 +308,11 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar address[] memory lIntermediateRoute = new address[](2); lIntermediateRoute[0] = lToken0; lIntermediateRoute[1] = lToken1; - uint16[] memory asd = new uint16[](1); - asd[0] = aBpMaxReward; - setRoute(lToken0, lToken1, lIntermediateRoute, asd); + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = aBpMaxReward; + setRoute(lToken0, lToken1, lIntermediateRoute, lRewardThreshold); + } else { + require(lData.isSimplePrice(), OracleErrors.AttemptToUseCompositeRouteAsIntermediateRoute()); } } @@ -361,10 +364,12 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar _getRouteDecimalDifferencePrice(lToken0, lToken1); if (lRoute.length == 0) { - // There is one case where the behavior is a bit more unexpected, and that is when - // `aBase` is an empty contract, and the revert would not be caught at all, causing - // the entire operation to fail. But this is okay, because if `aBase` is not a contract, trying - // to use the fallbackOracle would not yield any results anyway. + // There are two cases where the behavior is a bit more unexpected, and that is when: + // (1) `aBase` is an empty contract, and the revert would not be caught at all, causing + // the entire operation to fail. + // (2) an error happens when decoding the return data (specifically the return data length) + // But this is okay, because if `aBase` is not a contract or tries to return different types, + // trying to use the fallbackOracle would not yield any results anyway. // An alternative would be to use a low level `staticcall`. try IERC4626(aBase).asset() returns (address rBaseAsset) { uint256 lResolvedAmountIn = IERC4626(aBase).convertToAssets(aAmount); @@ -447,7 +452,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar emit FallbackOracleSet(aFallbackOracle); } - function updateTwapPeriod(uint64 aNewPeriod) public onlyOwner { + function updateTwapPeriod(uint16 aNewPeriod) public onlyOwner { require(aNewPeriod != 0 && aNewPeriod <= Constants.MAX_TWAP_PERIOD, OracleErrors.InvalidTwapPeriod()); twapPeriod = aNewPeriod; @@ -482,20 +487,27 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar /// @param aToken0 Address of the lower token. /// @param aToken1 Address of the higher token. /// @param aRoute Path with which the price between aToken0 and aToken1 should be derived. - /// @param aRewardThresholds Array of basis points at and beyond which a reward is applicable for a price update. - function setRoute(address aToken0, address aToken1, address[] memory aRoute, uint16[] memory aRewardThresholds) + /// @param aRewardThresholds Array of reward thresholds in WAD and beyond which a reward is applicable for a price update. + function setRoute(address aToken0, address aToken1, address[] memory aRoute, uint64[] memory aRewardThresholds) public onlyOwner { - uint256 lRouteLength = aRoute.length; - _validateTokens(aToken0, aToken1); + + uint256 lRouteLength = aRoute.length; require(lRouteLength > 1 && lRouteLength <= Constants.MAX_ROUTE_LENGTH, OracleErrors.InvalidRouteLength()); require(aRoute[0] == aToken0 && aRoute[lRouteLength - 1] == aToken1, OracleErrors.InvalidRoute()); require(aRewardThresholds.length == lRouteLength - 1, OracleErrors.InvalidRewardThresholdsLength()); bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1); + bytes32 lSlotData; + assembly ("memory-safe") { + lSlotData := sload(lSlot) + } + // clear existing route first if there is one + if (lSlotData != 0) clearRoute(aToken0, aToken1); + // simple route if (lRouteLength == 2) { uint256 lToken0Decimals = IERC20(aToken0).decimals(); @@ -506,7 +518,7 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar uint256 lRewardThreshold = aRewardThresholds[0]; require( - lRewardThreshold <= Constants.BP_SCALE && lRewardThreshold != 0, OracleErrors.InvalidRewardThreshold() + lRewardThreshold <= Constants.WAD && lRewardThreshold != 0, OracleErrors.InvalidRewardThreshold() ); bytes32 lData = RoutesLib.packSimplePrice(lDiff, 0, lRewardThreshold); @@ -543,7 +555,10 @@ contract ReservoirPriceOracle is IPriceOracle, Owned(msg.sender), ReentrancyGuar emit Route(aToken0, aToken1, aRoute); } - function clearRoute(address aToken0, address aToken1) external onlyOwner { + /// @notice Clears the defined route and the corresponding storage slots. + /// @param aToken0 Address of the lower token. + /// @param aToken1 Address of the higher token. + function clearRoute(address aToken0, address aToken1) public onlyOwner { _validateTokens(aToken0, aToken1); (address[] memory lRoute,,,) = _getRouteDecimalDifferencePrice(aToken0, aToken1); diff --git a/src/interfaces/IPriceOracle.sol b/src/interfaces/IPriceOracle.sol index adad550..c6f3d5c 100644 --- a/src/interfaces/IPriceOracle.sol +++ b/src/interfaces/IPriceOracle.sol @@ -2,23 +2,26 @@ pragma solidity ^0.8.0; interface IPriceOracle { + /// @notice Get the name of the oracle. + /// @return The name of the oracle. function name() external view returns (string memory); - /// @notice Returns the quote for a given amount of base asset in quote asset. - /// @param amount The amount of base asset. - /// @param base The address of the base asset. - /// @param quote The address of the quote asset. - /// @return out The quote amount in quote asset. - function getQuote(uint256 amount, address base, address quote) external view returns (uint256 out); + /// @notice One-sided price: How much quote token you would get for inAmount of base token, assuming no price + /// spread. + /// @param inAmount The amount of `base` to convert. + /// @param base The token that is being priced. + /// @param quote The token that is the unit of account. + /// @return outAmount The amount of `quote` that is equivalent to `inAmount` of `base`. + function getQuote(uint256 inAmount, address base, address quote) external view returns (uint256 outAmount); - /// @notice Returns the bid and ask quotes for a given amount of base asset in quote asset. - /// @param amount The amount of base asset. - /// @param base The address of the base asset. - /// @param quote The address of the quote asset. - /// @return bidOut The bid quote amount in quote asset. - /// @return askOut The ask quote amount in quote asset. - function getQuotes(uint256 amount, address base, address quote) + /// @notice Two-sided price: How much quote token you would get/spend for selling/buying inAmount of base token. + /// @param inAmount The amount of `base` to convert. + /// @param base The token that is being priced. + /// @param quote The token that is the unit of account. + /// @return bidOutAmount The amount of `quote` you would get for selling `inAmount` of `base`. + /// @return askOutAmount The amount of `quote` you would spend for buying `inAmount` of `base`. + function getQuotes(uint256 inAmount, address base, address quote) external view - returns (uint256 bidOut, uint256 askOut); + returns (uint256 bidOutAmount, uint256 askOutAmount); } diff --git a/src/libraries/Constants.sol b/src/libraries/Constants.sol index 175c3b9..d4bee05 100644 --- a/src/libraries/Constants.sol +++ b/src/libraries/Constants.sol @@ -6,11 +6,9 @@ library Constants { // CONSTANTS // /////////////////////////////////////////////////////////////////////////////////////////////// - uint256 public constant MAX_DEVIATION_THRESHOLD = 0.1e18; // 10% + uint64 public constant WAD = 1e18; uint256 public constant MAX_TWAP_PERIOD = 1 hours; uint256 public constant MAX_ROUTE_LENGTH = 4; - uint256 public constant WAD = 1e18; uint256 public constant MAX_SUPPORTED_PRICE = type(uint128).max; uint256 public constant MAX_AMOUNT_IN = type(uint128).max; - uint16 public constant BP_SCALE = 1e4; } diff --git a/src/libraries/OracleErrors.sol b/src/libraries/OracleErrors.sol index 95b3ca3..782f754 100644 --- a/src/libraries/OracleErrors.sol +++ b/src/libraries/OracleErrors.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.0; /// @dev Collection of all oracle related errors. library OracleErrors { // config errors + error AttemptToUseCompositeRouteAsIntermediateRoute(); error IncorrectTokensDesignatePair(); error InvalidRewardThreshold(); error InvalidRewardThresholdsLength(); @@ -12,7 +13,6 @@ library OracleErrors { error InvalidTokensProvided(); error InvalidTwapPeriod(); error NoDesignatedPair(); - error PriceDeviationThresholdTooHigh(); error UnsupportedTokenDecimals(); // query errors diff --git a/src/libraries/RoutesLib.sol b/src/libraries/RoutesLib.sol index cc8bbf8..956c95c 100644 --- a/src/libraries/RoutesLib.sol +++ b/src/libraries/RoutesLib.sol @@ -28,15 +28,15 @@ library RoutesLib { } // Assumes that aDecimalDifference is between -18 and 18 - // Assumes that aPrice is between 1 and 1e36 - // Assumes that aRewardThreshold is <= Constants.BP_SCALE + // Assumes that aPrice is between 1 and `Constants.MAX_SUPPORTED_PRICE` + // Assumes that aRewardThreshold is between 1 and `Constants.WAD` function packSimplePrice(int256 aDecimalDifference, uint256 aPrice, uint256 aRewardThreshold) internal pure returns (bytes32 rPacked) { bytes32 lDecimalDifferenceRaw = bytes1(uint8(int8(aDecimalDifference))); - bytes32 lRewardThreshold = bytes2(uint16(aRewardThreshold)); + bytes32 lRewardThreshold = bytes8(uint64(aRewardThreshold)); rPacked = FLAG_SIMPLE_PRICE | lDecimalDifferenceRaw >> 8 | lRewardThreshold >> 16 | bytes32(aPrice); } @@ -61,12 +61,12 @@ library RoutesLib { } function getPrice(bytes32 aData) internal pure returns (uint256 rPrice) { - rPrice = uint256(aData & 0x00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff); + rPrice = uint256(aData & 0x00000000000000000000ffffffffffffffffffffffffffffffffffffffffffff); } - function getRewardThreshold(bytes32 aData) internal pure returns (uint16 rRewardThreshold) { + function getRewardThreshold(bytes32 aData) internal pure returns (uint64 rRewardThreshold) { rRewardThreshold = - uint16(uint256((aData & 0x0000ffff00000000000000000000000000000000000000000000000000000000) >> 224)); + uint64(uint256((aData & 0x0000ffffffffffffffff00000000000000000000000000000000000000000000) >> 176)); } function getTokenFirstWord(bytes32 aData) internal pure returns (address rToken) { diff --git a/src/libraries/Utils.sol b/src/libraries/Utils.sol index f7ec76a..88f28fb 100644 --- a/src/libraries/Utils.sol +++ b/src/libraries/Utils.sol @@ -14,6 +14,7 @@ library Utils { /// @dev Assumes that `aOriginal` and `aNew` is less than or equal to /// `Constants.MAX_SUPPORTED_PRICE`. So multiplication by 1e18 will not overflow. + /// 1e18 indicates a 100% difference. i.e. a doubling in price function calcPercentageDiff(uint256 aOriginal, uint256 aNew) internal pure returns (uint256) { unchecked { if (aOriginal == 0) return 0; diff --git a/test/__fixtures/BaseTest.t.sol b/test/__fixtures/BaseTest.t.sol index 8ab1e97..ca9c22f 100644 --- a/test/__fixtures/BaseTest.t.sol +++ b/test/__fixtures/BaseTest.t.sol @@ -17,7 +17,7 @@ contract BaseTest is Test { using FactoryStoreLib for GenericFactory; uint64 internal constant DEFAULT_REWARD_GAS_AMOUNT = 200_000; - uint64 internal constant DEFAULT_TWAP_PERIOD = 15 minutes; + uint16 internal constant DEFAULT_TWAP_PERIOD = 15 minutes; GenericFactory internal _factory = new GenericFactory(); ReservoirPair internal _pair; diff --git a/test/large/ReservoirPriceOracleLarge.t.sol b/test/large/ReservoirPriceOracleLarge.t.sol index b0dd10f..67f045e 100644 --- a/test/large/ReservoirPriceOracleLarge.t.sol +++ b/test/large/ReservoirPriceOracleLarge.t.sol @@ -31,12 +31,24 @@ contract ReservoirPriceOracleLargeTest is ReservoirPriceOracleTest { ) external { // assume vm.assume( - aTokenAAddress > ADDRESS_THRESHOLD && aTokenBAddress > ADDRESS_THRESHOLD - && aTokenCAddress > ADDRESS_THRESHOLD && aTokenDAddress > ADDRESS_THRESHOLD + aTokenAAddress.code.length == 0 && aTokenBAddress.code.length == 0 && aTokenCAddress.code.length == 0 + && aTokenCAddress.code.length == 0 ); + assumeNotPrecompile(aTokenAAddress); + assumeNotPrecompile(aTokenBAddress); + assumeNotPrecompile(aTokenCAddress); + assumeNotPrecompile(aTokenDAddress); + assumeNotZeroAddress(aTokenAAddress); + assumeNotZeroAddress(aTokenBAddress); + assumeNotZeroAddress(aTokenCAddress); + assumeNotZeroAddress(aTokenDAddress); + assumeNotForgeAddress(aTokenAAddress); + assumeNotForgeAddress(aTokenBAddress); + assumeNotForgeAddress(aTokenCAddress); + assumeNotForgeAddress(aTokenDAddress); vm.assume( - _addressSet.add(aTokenAAddress) && _addressSet.add(aTokenBAddress) && _addressSet.add(aTokenCAddress) - && _addressSet.add(aTokenDAddress) + aTokenAAddress != aTokenBAddress && aTokenAAddress != aTokenCAddress && aTokenAAddress != aTokenDAddress + && aTokenBAddress != aTokenCAddress && aTokenBAddress != aTokenDAddress && aTokenBAddress != aTokenDAddress ); uint256 lPrice1 = bound(aPrice1, 1e12, 1e24); uint256 lPrice2 = bound(aPrice2, 1e12, 1e24); @@ -78,8 +90,8 @@ contract ReservoirPriceOracleLargeTest is ReservoirPriceOracleTest { lRoute[3] = aTokenAAddress; } - uint16[] memory lRewardThresholds = new uint16[](3); - lRewardThresholds[0] = lRewardThresholds[1] = lRewardThresholds[2] = Constants.BP_SCALE; + uint64[] memory lRewardThresholds = new uint64[](3); + lRewardThresholds[0] = lRewardThresholds[1] = lRewardThresholds[2] = Constants.WAD; _oracle.setRoute(lRoute[0], lRoute[3], lRoute, lRewardThresholds); _writePriceCache( diff --git a/test/unit/ReservoirPriceOracle.t.sol b/test/unit/ReservoirPriceOracle.t.sol index 74b7eb2..121ebf9 100644 --- a/test/unit/ReservoirPriceOracle.t.sol +++ b/test/unit/ReservoirPriceOracle.t.sol @@ -20,7 +20,6 @@ import { EnumerableSetLib } from "lib/solady/src/utils/EnumerableSetLib.sol"; import { Constants } from "src/libraries/Constants.sol"; import { MockFallbackOracle } from "test/mock/MockFallbackOracle.sol"; import { StubERC4626 } from "test/mock/StubERC4626.sol"; -import {Errors} from "../../lib/amm-core/test/integration/AaveErrors.sol"; contract ReservoirPriceOracleTest is BaseTest { using Utils for *; @@ -56,7 +55,7 @@ contract ReservoirPriceOracleTest is BaseTest { lDecimalDiff = lDecimalDiff == 0 ? int256(uint256(IERC20(aToken1).decimals())) - int256(uint256(IERC20(aToken0).decimals())) : lDecimalDiff; - bytes32 lData = lDecimalDiff.packSimplePrice(aPrice, uint16(lRewardThreshold)); + bytes32 lData = lDecimalDiff.packSimplePrice(aPrice, lRewardThreshold); require(lData.isSimplePrice(), "flag incorrect"); require(lData.getDecimalDifference() == lDecimalDiff, "decimal diff incorrect"); require(lData.getRewardThreshold() == lRewardThreshold, "reward threshold incorrect"); @@ -88,7 +87,7 @@ contract ReservoirPriceOracleTest is BaseTest { function setUp() external { // define route address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); + uint64[] memory lRewardThreshold = new uint64[](1); lRoute[0] = address(_tokenA); lRoute[1] = address(_tokenB); lRewardThreshold[0] = 200; // 2% @@ -99,7 +98,7 @@ contract ReservoirPriceOracleTest is BaseTest { _oracle.setRoute(address(_tokenA), address(_tokenB), lRoute, lRewardThreshold); } - function testName() external { + function testName() external view { // act & assert assertEq(_oracle.name(), "RESERVOIR PRICE ORACLE"); } @@ -160,8 +159,8 @@ contract ReservoirPriceOracleTest is BaseTest { _writePriceCache(address(_tokenC), address(_tokenD), lPriceCD); address[] memory lRoute = new address[](4); - uint16[] memory lRewardThreshold = new uint16[](3); - lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; lRoute[0] = address(_tokenA); lRoute[1] = address(_tokenB); @@ -190,8 +189,8 @@ contract ReservoirPriceOracleTest is BaseTest { _writePriceCache(address(_tokenC), address(_tokenD), lPriceCD); address[] memory lRoute = new address[](4); - uint16[] memory lRewardThreshold = new uint16[](3); - lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; lRoute[0] = address(_tokenA); lRoute[1] = address(_tokenB); lRoute[2] = address(_tokenC); @@ -233,8 +232,8 @@ contract ReservoirPriceOracleTest is BaseTest { lRoute[2] = address(lTokenC); { - uint16[] memory lRewardThreshold = new uint16[](2); - lRewardThreshold[0] = lRewardThreshold[1] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; _oracle.setRoute(address(lTokenA), address(lTokenC), lRoute, lRewardThreshold); _writePriceCache(address(lTokenA), address(lTokenB), 1e18); _writePriceCache(address(lTokenC), address(lTokenB), 1e18); @@ -260,8 +259,13 @@ contract ReservoirPriceOracleTest is BaseTest { uint8 aTokenBDecimal ) external { // assume - vm.assume(aTokenAAddress > ADDRESS_THRESHOLD && aTokenBAddress > ADDRESS_THRESHOLD); // avoid precompile addresses - vm.assume(_addressSet.add(aTokenAAddress) && _addressSet.add(aTokenBAddress)); + vm.assume(aTokenAAddress.code.length == 0 && aTokenBAddress.code.length == 0); + assumeNotPrecompile(aTokenAAddress); + assumeNotPrecompile(aTokenBAddress); + assumeNotZeroAddress(aTokenAAddress); + assumeNotZeroAddress(aTokenBAddress); + assumeNotForgeAddress(aTokenAAddress); + assumeNotForgeAddress(aTokenBAddress); uint256 lPrice = bound(aPrice, 1, 1e36); uint256 lAmtIn = bound(aAmtIn, 0, 1_000_000_000); uint256 lTokenADecimal = bound(aTokenADecimal, 0, 18); @@ -277,8 +281,8 @@ contract ReservoirPriceOracleTest is BaseTest { _oracle.designatePair(address(lTokenA), address(lTokenB), lPair); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; (lRoute[0], lRoute[1]) = lTokenA < lTokenB ? (address(lTokenA), address(lTokenB)) : (address(lTokenB), address(lTokenA)); _oracle.setRoute(lRoute[0], lRoute[1], lRoute, lRewardThreshold); @@ -307,11 +311,16 @@ contract ReservoirPriceOracleTest is BaseTest { uint8 aTokenCDecimal ) external { // assume - vm.assume( - aTokenAAddress > ADDRESS_THRESHOLD && aTokenBAddress > ADDRESS_THRESHOLD - && aTokenCAddress > ADDRESS_THRESHOLD - ); - vm.assume(_addressSet.add(aTokenAAddress) && _addressSet.add(aTokenBAddress) && _addressSet.add(aTokenCAddress)); + vm.assume(aTokenAAddress.code.length == 0 && aTokenBAddress.code.length == 0 && aTokenCAddress.code.length == 0); + assumeNotPrecompile(aTokenAAddress); + assumeNotPrecompile(aTokenBAddress); + assumeNotPrecompile(aTokenCAddress); + assumeNotZeroAddress(aTokenAAddress); + assumeNotZeroAddress(aTokenBAddress); + assumeNotZeroAddress(aTokenCAddress); + assumeNotForgeAddress(aTokenAAddress); + assumeNotForgeAddress(aTokenBAddress); + assumeNotForgeAddress(aTokenCAddress); uint256 lPrice1 = bound(aPrice1, 1e9, 1e25); // need to bound price within this range as a price below this will go to zero as during the mul and div of prices uint256 lPrice2 = bound(aPrice2, 1e9, 1e25); uint256 lAmtIn = bound(aAmtIn, 0, 1_000_000_000); @@ -339,8 +348,8 @@ contract ReservoirPriceOracleTest is BaseTest { lTokenA < lTokenC ? (address(lTokenA), address(lTokenC)) : (address(lTokenC), address(lTokenA)); lRoute[1] = address(lTokenB); - uint16[] memory lRewardThreshold = new uint16[](2); - lRewardThreshold[0] = lRewardThreshold[1] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; _oracle.setRoute(lRoute[0], lRoute[2], lRoute, lRewardThreshold); _writePriceCache( address(lTokenA) < address(lTokenB) ? address(lTokenA) : address(lTokenB), @@ -445,7 +454,7 @@ contract ReservoirPriceOracleTest is BaseTest { function testUpdateTwapPeriod(uint256 aNewPeriod) external { // assume - uint64 lNewPeriod = uint64(bound(aNewPeriod, 1, 1 hours)); + uint16 lNewPeriod = uint16(bound(aNewPeriod, 1, 1 hours)); // act _oracle.updateTwapPeriod(lNewPeriod); @@ -493,7 +502,7 @@ contract ReservoirPriceOracleTest is BaseTest { function testUpdatePrice_BelowThreshold(uint256 aPercentDiff) external { // assume (,, uint256 lRewardThreshold) = _oracle.priceCache(address(_tokenA), address(_tokenB)); - uint256 lPercentDiff = bound(aPercentDiff, 0, (lRewardThreshold - 1) * WAD / Constants.BP_SCALE); + uint256 lPercentDiff = bound(aPercentDiff, 0, (lRewardThreshold - 1)); // arrange - we fuzz test by varying the starting price instead of the new price uint256 lCurrentPrice = 98_918_868_099_219_913_512; @@ -518,12 +527,7 @@ contract ReservoirPriceOracleTest is BaseTest { function testUpdatePrice_AboveThresholdBelowMaxReward(uint256 aPercentDiff) external { // assume (,, uint256 lRewardThreshold) = _oracle.priceCache(address(_tokenA), address(_tokenB)); - uint256 lRewardThresholdWAD = lRewardThreshold * WAD / Constants.BP_SCALE; - uint256 lPercentDiff = bound( - aPercentDiff, - lRewardThresholdWAD, - _oracle.MAX_REWARD_MULTIPLIER() * lRewardThreshold * WAD / Constants.BP_SCALE - ); + uint256 lPercentDiff = bound(aPercentDiff, lRewardThreshold, _oracle.MAX_REWARD_MULTIPLIER() * lRewardThreshold); // arrange uint256 lCurrentPrice = 98_918_868_099_219_913_512; @@ -541,7 +545,7 @@ contract ReservoirPriceOracleTest is BaseTest { // assert (uint256 lPrice,,) = _oracle.priceCache(address(_tokenA), address(_tokenB)); assertEq(lPrice, lCurrentPrice); - uint256 lExpectedRewardReceived = block.basefee * _oracle.rewardGasAmount() * lPercentDiff / lRewardThresholdWAD; + uint256 lExpectedRewardReceived = block.basefee * _oracle.rewardGasAmount() * lPercentDiff / lRewardThreshold; assertGe(lExpectedRewardReceived, block.basefee * _oracle.rewardGasAmount()); assertLe(lExpectedRewardReceived, block.basefee * _oracle.rewardGasAmount() * _oracle.MAX_REWARD_MULTIPLIER()); assertEq(address(this).balance, lExpectedRewardReceived); // some reward received but is less than max possible reward @@ -553,7 +557,7 @@ contract ReservoirPriceOracleTest is BaseTest { (,, uint256 lRewardThreshold) = _oracle.priceCache(address(_tokenA), address(_tokenB)); uint256 lPercentDiff = bound( aPercentDiff, - _oracle.MAX_REWARD_MULTIPLIER() * lRewardThreshold * WAD / Constants.BP_SCALE, + _oracle.MAX_REWARD_MULTIPLIER() * lRewardThreshold, WAD // 100% ); @@ -649,7 +653,7 @@ contract ReservoirPriceOracleTest is BaseTest { address lIntermediate2 = address(_tokenD); address lEnd = address(_tokenB); address[] memory lRoute = new address[](4); - uint16[] memory lRewardThreshold = new uint16[](3); + uint64[] memory lRewardThreshold = new uint64[](3); lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = 3; // 3bp lRoute[0] = lStart; lRoute[1] = lIntermediate1; @@ -717,8 +721,8 @@ contract ReservoirPriceOracleTest is BaseTest { address lToken0 = address(_tokenB); address lToken1 = address(_tokenC); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = lToken0; lRoute[1] = lToken1; @@ -741,8 +745,8 @@ contract ReservoirPriceOracleTest is BaseTest { address lToken0 = address(_tokenB); address lToken1 = address(_tokenC); address[] memory lRoute = new address[](4); - uint16[] memory lRewardThreshold = new uint16[](3); - lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; lRoute[0] = lToken0; lRoute[1] = address(_tokenA); lRoute[2] = address(_tokenD); @@ -782,8 +786,8 @@ contract ReservoirPriceOracleTest is BaseTest { lIntermediateRoute3[0] = lIntermediate2; lIntermediateRoute3[1] = lEnd; - uint16[] memory lRewardThreshold = new uint16[](3); - lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; // act vm.expectEmit(false, false, false, true); @@ -805,13 +809,95 @@ contract ReservoirPriceOracleTest is BaseTest { assertEq(_oracle.route(lIntermediate2, lEnd), lIntermediateRoute3); } + function testSetRoute_ReplaceExistingCompositeWithSimple() external { + // arrange - set route A-B-C + address lStart = address(_tokenA); + address lIntermediate = address(_tokenB); + address lEnd = address(_tokenC); + address[] memory lRoute = new address[](3); + lRoute[0] = lStart; + lRoute[1] = lIntermediate; + lRoute[2] = lEnd; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; + _oracle.setRoute(lStart, lEnd, lRoute, lRewardThreshold); + + // act - overwrite by A-C-D + _oracle.clearRoute(lStart, lEnd); + lRoute[1] = address(_tokenC); + lRoute[2] = address(_tokenD); + + _oracle.setRoute(address(_tokenA), address(_tokenD), lRoute, lRewardThreshold); + + // assert + address[] memory lRouteAC = new address[](2); + lRouteAC[0] = address(_tokenA); + lRouteAC[1] = address(_tokenC); + + // intermediate routes are all intact + assertEq(_oracle.route(address(_tokenA), address(_tokenC)), lRouteAC); + assertEq(_oracle.route(address(_tokenA), address(_tokenD)), lRoute); + assertEq(_oracle.route(address(_tokenA), address(_tokenB)).length, 2); + assertEq(_oracle.route(address(_tokenB), address(_tokenC)).length, 2); + assertEq(_oracle.route(address(_tokenC), address(_tokenD)).length, 2); + } + + function testSetRoute_ReplaceExistingSimpleWithComposite() external { + // arrange + address lStart = address(_tokenA); + address lIntermediate = address(_tokenC); + address lEnd = address(_tokenB); + address[] memory lRoute = new address[](3); + lRoute[0] = lStart; + lRoute[1] = lIntermediate; + lRoute[2] = lEnd; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; + + // act - replace existing A-B with A-C-B + _oracle.setRoute(lStart, lEnd, lRoute, lRewardThreshold); + + // assert + assertEq(_oracle.route(address(_tokenA), address(_tokenB)).length, 3); + } + + function testSetRoute_2ndSlotClearedWhenReplacing3HopByShorterRoute() external { + // arrange - set A-D-B-C + address[] memory lRoute = new address[](4); + address lStart = address(_tokenA); + address lIntermediate1 = address(_tokenD); + address lIntermediate2 = address(_tokenB); + address lEnd = address(_tokenC); + lRoute[0] = lStart; + lRoute[1] = lIntermediate1; + lRoute[2] = lIntermediate2; + lRoute[3] = lEnd; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; + + _oracle.setRoute(lStart, lEnd, lRoute, lRewardThreshold); + + // act - replace it by A-C + address[] memory lNewRoute = new address[](2); + lNewRoute[0] = address(_tokenA); + lNewRoute[1] = address(_tokenC); + uint64[] memory lNewRewardThreshold = new uint64[](1); + lNewRewardThreshold[0] = Constants.WAD; + _oracle.setRoute(address(_tokenA), address(_tokenC), lNewRoute, lNewRewardThreshold); + + // assert + bytes32 lSecondSlot = bytes32(uint256(Utils.calculateSlot(address(_tokenA), address(_tokenC))) + 1); + bytes32 secondSlotValue = vm.load(address(_oracle), lSecondSlot); + assertEq(secondSlotValue, 0); + } + function testClearRoute() external { // arrange address lToken0 = address(_tokenB); address lToken1 = address(_tokenC); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = lToken0; lRoute[1] = lToken1; _oracle.setRoute(lToken0, lToken1, lRoute, lRewardThreshold); @@ -834,8 +920,8 @@ contract ReservoirPriceOracleTest is BaseTest { function testClearRoute_AllWordsCleared() external { // arrange address[] memory lRoute = new address[](4); - uint16[] memory lRewardThreshold = new uint16[](3); - lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](3); + lRewardThreshold[0] = lRewardThreshold[1] = lRewardThreshold[2] = Constants.WAD; lRoute[0] = address(_tokenA); lRoute[1] = address(_tokenC); lRoute[2] = address(_tokenB); @@ -939,7 +1025,7 @@ contract ReservoirPriceOracleTest is BaseTest { function testUpdateTwapPeriod_InvalidTwapPeriod(uint256 aNewPeriod) external { // assume - uint64 lNewPeriod = uint64(bound(aNewPeriod, 1 hours + 1, type(uint64).max)); + uint16 lNewPeriod = uint16(bound(aNewPeriod, 1 hours + 1, type(uint16).max)); // act & assert vm.expectRevert(OracleErrors.InvalidTwapPeriod.selector); @@ -961,8 +1047,8 @@ contract ReservoirPriceOracleTest is BaseTest { lPair.sync(); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = address(_tokenB); lRoute[1] = address(_tokenC); @@ -981,7 +1067,7 @@ contract ReservoirPriceOracleTest is BaseTest { function testUpdatePrice_WriteToNonSimpleRoute() external { // arrange - uint16[] memory lRewardThresholds = new uint16[](2); + uint64[] memory lRewardThresholds = new uint64[](2); lRewardThresholds[0] = lRewardThresholds[1] = 1; address[] memory lRoute = new address[](3); lRoute[0] = address(_tokenA); @@ -1021,8 +1107,8 @@ contract ReservoirPriceOracleTest is BaseTest { address lToken0 = address(0x1); address lToken1 = address(0x1); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = lToken0; lRoute[1] = lToken1; @@ -1036,8 +1122,8 @@ contract ReservoirPriceOracleTest is BaseTest { address lToken0 = address(0x21); address lToken1 = address(0x2); address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = lToken0; lRoute[1] = lToken1; @@ -1051,8 +1137,8 @@ contract ReservoirPriceOracleTest is BaseTest { address lToken0 = address(0x1); address lToken1 = address(0x2); address[] memory lTooLong = new address[](5); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; lTooLong[0] = lToken0; lTooLong[1] = address(0); lTooLong[2] = address(0); @@ -1084,8 +1170,8 @@ contract ReservoirPriceOracleTest is BaseTest { lInvalidRoute2[1] = address(54); lInvalidRoute2[2] = lToken1; - uint16[] memory lRewardThreshold = new uint16[](2); - lRewardThreshold[0] = lRewardThreshold[1] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; // act & assert vm.expectRevert(OracleErrors.InvalidRoute.selector); @@ -1097,8 +1183,8 @@ contract ReservoirPriceOracleTest is BaseTest { function testSetRoute_InvalidRewardThreshold() external { // arrange address[] memory lRoute = new address[](2); - uint16[] memory lInvalidRewardThreshold = new uint16[](1); - lInvalidRewardThreshold[0] = Constants.BP_SCALE + 1; + uint64[] memory lInvalidRewardThreshold = new uint64[](1); + lInvalidRewardThreshold[0] = Constants.WAD + 1; lRoute[0] = address(_tokenC); lRoute[1] = address(_tokenD); @@ -1113,8 +1199,8 @@ contract ReservoirPriceOracleTest is BaseTest { function testSetRoute_InvalidRewardThresholdLength() external { address[] memory lRoute = new address[](2); - uint16[] memory lRewardThreshold = new uint16[](2); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = Constants.WAD; lRoute[0] = address(_tokenC); lRoute[1] = address(_tokenD); @@ -1129,8 +1215,8 @@ contract ReservoirPriceOracleTest is BaseTest { address[] memory lRoute = new address[](2); lRoute[0] = address(lToken) < address(_tokenA) ? address(lToken) : address(_tokenA); lRoute[1] = address(lToken) < address(_tokenA) ? address(_tokenA) : address(lToken); - uint16[] memory lRewardThreshold = new uint16[](1); - lRewardThreshold[0] = Constants.BP_SCALE; + uint64[] memory lRewardThreshold = new uint64[](1); + lRewardThreshold[0] = Constants.WAD; // act & assert vm.expectRevert(OracleErrors.UnsupportedTokenDecimals.selector); @@ -1142,6 +1228,36 @@ contract ReservoirPriceOracleTest is BaseTest { ); } + // modified from test case supplied by Cantina auditors + function testSetRoute_OverwriteExisting_UsingCompositeAsIntermediate() external { + // arrange - set pair A/C with route [A->B->C] + address lStart = address(_tokenA); + address lIntermediate1 = address(_tokenB); + address lEnd = address(_tokenC); + address[] memory lRoute = new address[](3); + lRoute[0] = lStart; + lRoute[1] = lIntermediate1; + lRoute[2] = lEnd; + uint64[] memory lRewardThreshold = new uint64[](2); + lRewardThreshold[0] = lRewardThreshold[1] = Constants.WAD; + _oracle.setRoute(lStart, lEnd, lRoute, lRewardThreshold); + + // Test if A/C slot contains 2-HOP route as expected + bytes32 AC_Slot = Utils.calculateSlot(address(_tokenA), address(_tokenC)); + bytes32 AC_SlotValue = vm.load(address(_oracle), AC_Slot); + assertEq(AC_SlotValue.is2HopRoute(), true); + + // act - Overwrite with pair A/D with route [A->C->D] + lEnd = address(_tokenD); + lRoute = new address[](3); + lRoute[0] = lStart; + lRoute[1] = address(_tokenC); + lRoute[2] = lEnd = address(_tokenD); + + vm.expectRevert(OracleErrors.AttemptToUseCompositeRouteAsIntermediateRoute.selector); + _oracle.setRoute(lStart, lEnd, lRoute, lRewardThreshold); + } + function testUpdateRewardGasAmount_NotOwner() external { // act & assert vm.prank(address(123)); diff --git a/test/unit/libraries/RoutesLib.t.sol b/test/unit/libraries/RoutesLib.t.sol index df0540c..f764738 100644 --- a/test/unit/libraries/RoutesLib.t.sol +++ b/test/unit/libraries/RoutesLib.t.sol @@ -36,17 +36,17 @@ contract RoutesLibTest is Test { assertEq(lZero.getDecimalDifference(), 0); } - function testPackSimplePrice(int8 aDiff, uint256 aPrice) external pure { + function testPackSimplePrice(int8 aDiff, uint64 aRewardThreshold, uint256 aPrice) external pure { // assume - uint256 lPrice = bound(aPrice, 1, 1e36); + uint256 lPrice = bound(aPrice, 1, Constants.MAX_SUPPORTED_PRICE); // act - bytes32 lResult = int256(aDiff).packSimplePrice(lPrice, Constants.BP_SCALE); + bytes32 lResult = int256(aDiff).packSimplePrice(lPrice, aRewardThreshold); // assert assertEq(lResult[0], RoutesLib.FLAG_SIMPLE_PRICE); assertEq(lResult[1], bytes1(uint8(aDiff))); - assertEq(lResult.getRewardThreshold(), Constants.BP_SCALE); + assertEq(lResult.getRewardThreshold(), aRewardThreshold); assertEq(lResult.getPrice(), lPrice); } } diff --git a/test/unit/libraries/Utils.t.sol b/test/unit/libraries/Utils.t.sol new file mode 100644 index 0000000..6142824 --- /dev/null +++ b/test/unit/libraries/Utils.t.sol @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.10; + +import { Test, console2, stdError } from "forge-std/Test.sol"; + +import { Utils } from "src/libraries/Utils.sol"; +import { Constants } from "src/libraries/Constants.sol"; + +contract UtilsTest is Test{ + using Utils for uint256; + + function testCalcPercentageDiff_Double(uint256 aOriginal) external pure { + // assume + uint256 lOriginal = bound(aOriginal, 1, Constants.MAX_SUPPORTED_PRICE / 2); + + // act + uint256 lDiff = lOriginal.calcPercentageDiff( lOriginal * 2); + + // assert + assertEq(lDiff, 1e18); + } + + function testCalcPercentageDiff_Half(uint256 aOriginal) external pure { + // assume - when numbers get too small the error becomes too large + uint256 lOriginal = bound(aOriginal, 1e6, Constants.MAX_SUPPORTED_PRICE); + + // act + uint256 lDiff = lOriginal.calcPercentageDiff( lOriginal / 2); + + // assert + assertApproxEqRel(lDiff, 0.5e18, 0.00001e18); + } + + function testCalcPercentageDiff_NoDiff(uint256 aOriginal) external pure { + // assume + uint256 lOriginal = bound(aOriginal, 1, Constants.MAX_SUPPORTED_PRICE); + + // act + uint256 lDiff = lOriginal.calcPercentageDiff(lOriginal); + + // assert + assertEq(lDiff, 0); + } +}