fix: review comments

* questions * fix: make sure ETH transfer call never reverts * fix: all assembly blocks memory safe * docs: update comment * docs: update comment * fix: use pop in after low level call to clear stack * lint: add solhint disable * fix: rm review comment * fix: rm test case that wastes all gas --------- Co-authored-by: A.L. <[email protected]>
reservoir-labs · Jul 8, 2024 · b353d0b · b353d0b
1 parent f7a709c
commit b353d0b
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 45 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -1,19 +1,19 @@
-QueryProcessorTest:testFindNearestSample_CanFindExactValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 67585746, ~: 76007063)
-QueryProcessorTest:testFindNearestSample_CanFindIntermediateValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 67051023, ~: 76087612)
+QueryProcessorTest:testFindNearestSample_CanFindExactValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 66680532, ~: 75107672)
+QueryProcessorTest:testFindNearestSample_CanFindIntermediateValue(uint32,uint256,uint256,uint256) (runs: 256, μ: 65178546, ~: 75238528)
 QueryProcessorTest:testFindNearestSample_NotInitialized() (gas: 1056945756)
-QueryProcessorTest:testFindNearestSample_OneSample(uint256) (runs: 256, μ: 80325, ~: 80360)
+QueryProcessorTest:testFindNearestSample_OneSample(uint256) (runs: 256, μ: 80327, ~: 80360)
 QueryProcessorTest:testGetInstantValue() (gas: 124248)
 QueryProcessorTest:testGetInstantValue_NotInitialized(uint256) (runs: 256, μ: 19397, ~: 19397)
-QueryProcessorTest:testGetInstantValue_NotInitialized_BeyondBufferSize(uint8,uint16) (runs: 256, μ: 68389647, ~: 68389600)
-QueryProcessorTest:testGetPastAccumulator_BufferEmpty(uint8) (runs: 256, μ: 27017, ~: 27087)
-QueryProcessorTest:testGetPastAccumulator_ExactMatch(uint32,uint256,uint256,uint16) (runs: 256, μ: 69289018, ~: 79307245)
-QueryProcessorTest:testGetPastAccumulator_ExactMatch_LatestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 70900090, ~: 79224986)
-QueryProcessorTest:testGetPastAccumulator_ExactMatch_OldestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 70930212, ~: 79256586)
-QueryProcessorTest:testGetPastAccumulator_ExtrapolatesBeyondLatest(uint32,uint256,uint256,uint256) (runs: 256, μ: 67024575, ~: 76058923)
-QueryProcessorTest:testGetPastAccumulator_InterpolatesBetweenPastAccumulators(uint32,uint256,uint256,uint256) (runs: 256, μ: 67058609, ~: 76093620)
-QueryProcessorTest:testGetPastAccumulator_InvalidAgo(uint32,uint256,uint256,uint256) (runs: 256, μ: 67016184, ~: 76050634)
-QueryProcessorTest:testGetPastAccumulator_QueryTooOld(uint32,uint256,uint256,uint256) (runs: 256, μ: 67027596, ~: 76060517)
-QueryProcessorTest:testGetTimeWeightedAverage(uint32,uint256,uint256,uint256,uint256) (runs: 256, μ: 101761121, ~: 108597954)
+QueryProcessorTest:testGetInstantValue_NotInitialized_BeyondBufferSize(uint8,uint16) (runs: 256, μ: 68389663, ~: 68389600)
+QueryProcessorTest:testGetPastAccumulator_BufferEmpty(uint8) (runs: 256, μ: 27027, ~: 27087)
+QueryProcessorTest:testGetPastAccumulator_ExactMatch(uint32,uint256,uint256,uint16) (runs: 256, μ: 72569441, ~: 82408620)
+QueryProcessorTest:testGetPastAccumulator_ExactMatch_LatestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 68439350, ~: 79368830)
+QueryProcessorTest:testGetPastAccumulator_ExactMatch_OldestAccumulator(uint32,uint256,uint256) (runs: 256, μ: 68469161, ~: 79400430)
+QueryProcessorTest:testGetPastAccumulator_ExtrapolatesBeyondLatest(uint32,uint256,uint256,uint256) (runs: 256, μ: 65152191, ~: 75209746)
+QueryProcessorTest:testGetPastAccumulator_InterpolatesBetweenPastAccumulators(uint32,uint256,uint256,uint256) (runs: 256, μ: 65186262, ~: 75244536)
+QueryProcessorTest:testGetPastAccumulator_InvalidAgo(uint32,uint256,uint256,uint256) (runs: 256, μ: 65143780, ~: 75201457)
+QueryProcessorTest:testGetPastAccumulator_QueryTooOld(uint32,uint256,uint256,uint256) (runs: 256, μ: 65155327, ~: 75211341)
+QueryProcessorTest:testGetTimeWeightedAverage(uint32,uint256,uint256,uint256,uint256) (runs: 256, μ: 104921242, ~: 114642503)
 QueryProcessorTest:testGetTimeWeightedAverage_BadSecs() (gas: 10995)
 ReservoirPriceOracleTest:testClearRoute() (gas: 50974)
 ReservoirPriceOracleTest:testClearRoute_AllWordsCleared() (gas: 151907)
@@ -23,24 +23,24 @@ ReservoirPriceOracleTest:testDesignatePair_NotOwner() (gas: 17531)
 ReservoirPriceOracleTest:testDesignatePair_TokenOrderReversed() (gas: 30729)
 ReservoirPriceOracleTest:testGasBountyAvailable(uint256) (runs: 256, μ: 9929, ~: 9925)
 ReservoirPriceOracleTest:testGasBountyAvailable_Zero() (gas: 8961)
-ReservoirPriceOracleTest:testGetLatest(uint32) (runs: 256, μ: 92685, ~: 92614)
+ReservoirPriceOracleTest:testGetLatest(uint32) (runs: 256, μ: 92675, ~: 92614)
 ReservoirPriceOracleTest:testGetLatest_Inverted() (gas: 96786)
-ReservoirPriceOracleTest:testGetQuote(uint256,uint256) (runs: 256, μ: 35198, ~: 35302)
+ReservoirPriceOracleTest:testGetQuote(uint256,uint256) (runs: 256, μ: 35190, ~: 35302)
 ReservoirPriceOracleTest:testGetQuote_AmountInTooLarge() (gas: 12963)
-ReservoirPriceOracleTest:testGetQuote_BaseIsVault(uint256) (runs: 256, μ: 417430, ~: 417188)
+ReservoirPriceOracleTest:testGetQuote_BaseIsVault(uint256) (runs: 256, μ: 417427, ~: 417188)
 ReservoirPriceOracleTest:testGetQuote_ComplicatedDecimals() (gas: 10350840)
-ReservoirPriceOracleTest:testGetQuote_Inverse(uint256,uint256) (runs: 256, μ: 37311, ~: 37474)
+ReservoirPriceOracleTest:testGetQuote_Inverse(uint256,uint256) (runs: 256, μ: 37300, ~: 37473)
 ReservoirPriceOracleTest:testGetQuote_MultipleHops() (gas: 113391)
 ReservoirPriceOracleTest:testGetQuote_MultipleHops_Inverse() (gas: 113646)
 ReservoirPriceOracleTest:testGetQuote_MultipleHops_PriceZero() (gas: 125259)
 ReservoirPriceOracleTest:testGetQuote_NoFallbackOracle() (gas: 20875)
 ReservoirPriceOracleTest:testGetQuote_PriceZero() (gas: 15902)
-ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_1HopRoute(uint256,uint256,address,address,uint8,uint8) (runs: 256, μ: 5327381, ~: 5327411)
-ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_2HopRoute(uint256,uint256,uint256,address,address,address,uint8,uint8,uint8) (runs: 256, μ: 10492723, ~: 10492862)
+ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_1HopRoute(uint256,uint256,address,address,uint8,uint8) (runs: 256, μ: 5327401, ~: 5327488)
+ReservoirPriceOracleTest:testGetQuote_RandomizeAllParam_2HopRoute(uint256,uint256,uint256,address,address,address,uint8,uint8,uint8) (runs: 256, μ: 10492876, ~: 10492862)
 ReservoirPriceOracleTest:testGetQuote_SameBaseQuote(uint256,address) (runs: 256, μ: 8963, ~: 8963)
 ReservoirPriceOracleTest:testGetQuote_UseFallback() (gas: 38312)
 ReservoirPriceOracleTest:testGetQuote_ZeroIn() (gas: 38147)
-ReservoirPriceOracleTest:testGetQuotes(uint256,uint256) (runs: 256, μ: 32687, ~: 32791)
+ReservoirPriceOracleTest:testGetQuotes(uint256,uint256) (runs: 256, μ: 32679, ~: 32791)
 ReservoirPriceOracleTest:testGetTimeWeightedAverage() (gas: 141765)
 ReservoirPriceOracleTest:testGetTimeWeightedAverage_Inverted() (gas: 120958)
 ReservoirPriceOracleTest:testSetFallbackOracle_NotOwner() (gas: 10938)
@@ -54,18 +54,18 @@ ReservoirPriceOracleTest:testSetRoute_SameToken() (gas: 12115)
 ReservoirPriceOracleTest:testUndesignatePair() (gas: 30257)
 ReservoirPriceOracleTest:testUndesignatePair_NotOwner() (gas: 15288)
 ReservoirPriceOracleTest:testUpdatePriceDeviationThreshold(uint256) (runs: 256, μ: 21328, ~: 21085)
-ReservoirPriceOracleTest:testUpdatePrice_BeyondThreshold() (gas: 213770)
-ReservoirPriceOracleTest:testUpdatePrice_BeyondThreshold_InsufficientReward(uint256) (runs: 256, μ: 202989, ~: 203067)
+ReservoirPriceOracleTest:testUpdatePrice_BeyondThreshold() (gas: 213669)
+ReservoirPriceOracleTest:testUpdatePrice_BeyondThreshold_InsufficientReward(uint256) (runs: 256, μ: 209788, ~: 210004)
 ReservoirPriceOracleTest:testUpdatePrice_BeyondThreshold_ZeroRecipient() (gas: 195593)
 ReservoirPriceOracleTest:testUpdatePrice_FirstUpdate() (gas: 203220)
 ReservoirPriceOracleTest:testUpdatePrice_IntermediateRoutes() (gas: 15867847)
 ReservoirPriceOracleTest:testUpdatePrice_PriceOutOfRange() (gas: 5350717)
 ReservoirPriceOracleTest:testUpdatePrice_WithinThreshold() (gas: 204109)
 ReservoirPriceOracleTest:testUpdateRewardGasAmount() (gas: 19033)
 ReservoirPriceOracleTest:testUpdateRewardGasAmount_NotOwner() (gas: 10984)
-ReservoirPriceOracleTest:testUpdateTwapPeriod(uint256) (runs: 256, μ: 21745, ~: 21828)
-ReservoirPriceOracleTest:testUpdateTwapPeriod_InvalidTwapPeriod(uint256) (runs: 256, μ: 17861, ~: 18164)
-ReservoirPriceOracleTest:testWritePriceCache(uint256) (runs: 256, μ: 29939, ~: 29697)
+ReservoirPriceOracleTest:testUpdateTwapPeriod(uint256) (runs: 256, μ: 21751, ~: 21828)
+ReservoirPriceOracleTest:testUpdateTwapPeriod_InvalidTwapPeriod(uint256) (runs: 256, μ: 17852, ~: 18164)
+ReservoirPriceOracleTest:testWritePriceCache(uint256) (runs: 256, μ: 29936, ~: 29697)
 RoutesLibTest:testGetDecimalDifference() (gas: 3974)
 RoutesLibTest:testIsCompositeRoute() (gas: 4341)
 RoutesLibTest:testPackSimplePrice(int8,uint256) (runs: 256, μ: 7786, ~: 7555)

diff --git a/src/ReservoirPriceOracle.sol b/src/ReservoirPriceOracle.sol
@@ -5,11 +5,7 @@ import { IERC20 } from "forge-std/interfaces/IERC20.sol";
 import { IERC4626 } from "forge-std/interfaces/IERC4626.sol";
 
 import { OracleErrors } from "src/libraries/OracleErrors.sol";
-import {
-    IReservoirPriceOracle,
-    OracleAverageQuery,
-    OracleLatestQuery
-} from "src/interfaces/IReservoirPriceOracle.sol";
+import { IReservoirPriceOracle, OracleAverageQuery, OracleLatestQuery } from "src/interfaces/IReservoirPriceOracle.sol";
 import { IPriceOracle } from "src/interfaces/IPriceOracle.sol";
 import { QueryProcessor, ReservoirPair, PriceType } from "src/libraries/QueryProcessor.sol";
 import { Utils } from "src/libraries/Utils.sol";
@@ -153,7 +149,9 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
             );
 
             // if it's a simple route, we avoid loading the price again from storage
-            if (lRoute.length != 2) (lPrevPrice,) = _priceCache(lToken0, lToken1);
+            if (lRoute.length != 2) {
+                (lPrevPrice,) = _priceCache(lToken0, lToken1);
+            }
 
             _writePriceCache(lToken0, lToken1, lNewPrice);
 
@@ -238,10 +236,10 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
             lPayoutAmt = block.basefee * rewardGasAmount;
         }
 
-        if (lPayoutAmt <= address(this).balance) {
-            payable(aRecipient).transfer(lPayoutAmt);
+        // does not revert under any circumstance
+        assembly ("memory-safe") {
+            pop(call(gas(), aRecipient, lPayoutAmt, codesize(), 0x00, codesize(), 0x00))
         }
-        // do nothing if lPayoutAmt is greater than the balance
     }
 
     /// @return rRoute The route to determine the price between aToken0 and aToken1
@@ -255,7 +253,7 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
         bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1);
 
         bytes32 lFirstWord;
-        assembly {
+        assembly ("memory-safe") {
             lFirstWord := sload(lSlot)
         }
 
@@ -277,7 +275,7 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
             } else {
                 assert(lFirstWord.is3HopRoute());
                 bytes32 lSecondWord;
-                assembly {
+                assembly ("memory-safe") {
                     lSecondWord := sload(add(lSlot, 1))
                 }
                 address lThirdToken = lSecondWord.getThirdToken();
@@ -303,7 +301,7 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
 
         bytes32 lSlot = Utils.calculateSlot(lToken0, lToken1);
         bytes32 lData;
-        assembly {
+        assembly ("memory-safe") {
             lData := sload(lSlot)
         }
         if (lData == bytes32(0)) {
@@ -323,7 +321,7 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
         bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1);
 
         bytes32 lData;
-        assembly {
+        assembly ("memory-safe") {
             lData := sload(lSlot)
         }
         if (lData.isSimplePrice()) {
@@ -337,15 +335,15 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
 
         bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1);
         bytes32 lData;
-        assembly {
+        assembly ("memory-safe") {
             lData := sload(lSlot)
         }
         if (!lData.isSimplePrice()) revert OracleErrors.WriteToNonSimpleRoute();
 
         int256 lDiff = lData.getDecimalDifference();
 
         lData = RoutesLib.packSimplePrice(lDiff, aNewPrice);
-        assembly {
+        assembly ("memory-safe") {
             sstore(lSlot, lData)
         }
     }
@@ -511,7 +509,7 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
             int256 lDiff = int256(lToken1Decimals) - int256(lToken0Decimals);
 
             bytes32 lData = RoutesLib.packSimplePrice(lDiff, 0);
-            assembly {
+            assembly ("memory-safe") {
                 // Write data to storage.
                 sstore(lSlot, lData)
             }
@@ -523,14 +521,14 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
 
             if (lRouteLength == 3) {
                 bytes32 lData = RoutesLib.pack2HopRoute(lSecondToken);
-                assembly {
+                assembly ("memory-safe") {
                     sstore(lSlot, lData)
                 }
             } else if (lRouteLength == 4) {
                 (bytes32 lFirstWord, bytes32 lSecondWord) = RoutesLib.pack3HopRoute(lSecondToken, lThirdToken);
 
                 // Write two words to storage.
-                assembly {
+                assembly ("memory-safe") {
                     sstore(lSlot, lFirstWord)
                     sstore(add(lSlot, 1), lSecondWord)
                 }
@@ -550,14 +548,14 @@ contract ReservoirPriceOracle is IPriceOracle, IReservoirPriceOracle, Owned(msg.
         bytes32 lSlot = Utils.calculateSlot(aToken0, aToken1);
 
         // clear the storage slot that the route has written to previously
-        assembly {
+        assembly ("memory-safe") {
             sstore(lSlot, 0)
         }
 
         // routes with length MAX_ROUTE_LENGTH use one more word of storage
         // `setRoute` enforces the MAX_ROUTE_LENGTH limit.
         if (lRoute.length == Constants.MAX_ROUTE_LENGTH) {
-            assembly {
+            assembly ("memory-safe") {
                 sstore(add(lSlot, 1), 0)
             }
         }