Merge pull request #131 from cryptoalgebra/fix/bailsec-audit-integral…

…v1.2

Fixes for BailSec audit of Integral v1.2

src/core/contracts/AlgebraFactory.sol

@@ -57,7 +57,7 @@ contract AlgebraFactory is IAlgebraFactory, Ownable2Step, AccessControlEnumerabl
 
   /// @inheritdoc IAlgebraFactory
   /// @dev keccak256 of AlgebraPool init bytecode. Used to compute pool address deterministically
-  bytes32 public constant POOL_INIT_CODE_HASH = 0x53a254b73c7f4f4a23175de0908ad4b30f3bc60806bd69bba905db6f24b991a5;
+  bytes32 public constant POOL_INIT_CODE_HASH = 0x3093a65c28d1e6def42997fdea76d033dfd42b432c107e19412cf91a1eac0f91;
 
   constructor(address _poolDeployer) {
     require(_poolDeployer != address(0));

src/core/contracts/AlgebraPool.sol

@@ -143,16 +143,19 @@ contract AlgebraPool is AlgebraPoolBase, TickStructure, ReentrancyGuard, Positio
       (amount0, amount1) = _updatePositionTicksAndFees(position, bottomTick, topTick, liquidityDelta);
 
       if (pluginFee > 0) {
+        uint256 deltaPluginFeePending0;
+        uint256 deltaPluginFeePending1;
+
         if (amount0 > 0) {
-          uint256 deltaPluginFeePending0 = FullMath.mulDiv(amount0, pluginFee, Constants.FEE_DENOMINATOR);
+          deltaPluginFeePending0 = FullMath.mulDiv(amount0, pluginFee, Constants.FEE_DENOMINATOR);
           amount0 -= deltaPluginFeePending0;
-          pluginFeePending0 += uint104(deltaPluginFeePending0);
         }
         if (amount1 > 0) {
-          uint256 deltaPluginFeePending1 = FullMath.mulDiv(amount1, pluginFee, Constants.FEE_DENOMINATOR);
+          deltaPluginFeePending1 = FullMath.mulDiv(amount1, pluginFee, Constants.FEE_DENOMINATOR);
           amount1 -= deltaPluginFeePending1;
-          pluginFeePending1 += uint104(deltaPluginFeePending1);
         }
+
+        _changeReserves(0, 0, 0, 0, deltaPluginFeePending0, deltaPluginFeePending1);
       }
 
       if (amount0 | amount1 != 0) {
@@ -370,7 +373,7 @@ contract AlgebraPool is AlgebraPoolBase, TickStructure, ReentrancyGuard, Positio
       if (_isPlugin()) return (0, 0);
       bytes4 selector;
       (selector, overrideFee, pluginFee) = IAlgebraPlugin(plugin).beforeSwap(msg.sender, recipient, zto, amount, limitPrice, payInAdvance, data);
-      if (overrideFee >= 1e6 || pluginFee > overrideFee) revert incorrectPluginFee();
+      // we will check that fee is less than denominator inside the swap calculation
       selector.shouldReturn(IAlgebraPlugin.beforeSwap.selector);
     }
   }

src/core/contracts/base/ReservesManager.sol

@@ -2,13 +2,15 @@
 pragma solidity =0.8.20;
 
 import '../libraries/SafeCast.sol';
+import '../libraries/Plugins.sol';
 import './AlgebraPoolBase.sol';
 import '../interfaces/plugin/IAlgebraPlugin.sol';
 import '../interfaces/pool/IAlgebraPoolErrors.sol';
 /// @title Algebra reserves management abstract contract
 /// @notice Encapsulates logic for tracking and changing pool reserves
 /// @dev The reserve mechanism allows the pool to keep track of unexpected increases in balances
 abstract contract ReservesManager is AlgebraPoolBase {
+  using Plugins for bytes4;
   using SafeCast for uint256;
 
   /// @dev The tracked token0 and token1 reserves of pool
@@ -67,55 +69,98 @@ abstract contract ReservesManager is AlgebraPoolBase {
     }
   }
 
-  function updateFeeAmounts(
-    int256 deltaR0,
-    int256 deltaR1,
+  /// @notice Accrues fees and transfers them to `recipient`
+  /// @dev If we transfer fees, writes zeros to the storage slot specified by the slot argument
+  /// If we do not transfer fees, returns actual pendingFees
+  function _accrueAndTransferFees(
     uint256 fee0,
     uint256 fee1,
-    address feesRecipient,
-    bytes32 slot,
-    uint32 lastTimestamp
-  ) internal returns (int256, int256, uint256, uint256) {
-    uint256 feePending0;
-    uint256 feePending1;
-
-    assembly {
-      // Load the storage slot specified by the slot argument
-      let sl := sload(slot)
-
-      // Extract the uint104 value
-      feePending0 := and(sl, 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
-
-      // Shift right by 104 bits and extract the uint104 value
-      feePending1 := and(shr(104, sl), 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
-    }
+    uint256 lastTimestamp,
+    bytes32 receiverSlot,
+    bytes32 feePendingSlot
+  ) internal returns (uint104, uint104, uint256, uint256) {
+    if (fee0 | fee1 != 0) {
+      uint256 feePending0;
+      uint256 feePending1;
+      assembly {
+        // Load the storage slot specified by the slot argument
+        let sl := sload(feePendingSlot)
+        // Extract the uint104 value
+        feePending0 := and(sl, 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
+        // Shift right by 104 bits and extract the uint104 value
+        feePending1 := and(shr(104, sl), 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
+      }
+      feePending0 += fee0;
+      feePending1 += fee1;
+
+      if (
+        _blockTimestamp() - lastTimestamp >= Constants.FEE_TRANSFER_FREQUENCY || feePending0 > type(uint104).max || feePending1 > type(uint104).max
+      ) {
+        // use sload from slot (like pointer dereference) to avoid gas
+        address recipient;
+        assembly {
+          recipient := sload(receiverSlot)
+        }
+        (uint256 feeSent0, uint256 feeSent1) = _transferFees(feePending0, feePending1, recipient);
+        // use sload from slot (like pointer dereference) to avoid gas
+        // override `lastFeeTransferTimestamp` with zeros is OK
+        // because we will update it later
+        assembly {
+          sstore(feePendingSlot, 0)
+        }
+        // sent fees return 0 pending and sent fees
+        return (0, 0, feeSent0, feeSent1);
+      } else {
+        // didn't send fees return pending fees and 0 sent
+        return (uint104(feePending0), uint104(feePending1), 0, 0);
+      }
+    } else {
+      if (_blockTimestamp() - lastTimestamp >= Constants.FEE_TRANSFER_FREQUENCY) {
+        uint256 feePending0;
+        uint256 feePending1;
+        assembly {
+          // Load the storage slot specified by the slot argument
+          let sl := sload(feePendingSlot)
+          // Extract the uint104 value
+          feePending0 := and(sl, 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
+          // Shift right by 104 bits and extract the uint104 value
+          feePending1 := and(shr(104, sl), 0xFFFFFFFFFFFFFFFFFFFFFFFFFF)
+        }
 
-    feePending0 += fee0;
-    feePending1 += fee1;
+        if (feePending0 | feePending1 != 0) {
+          address recipient;
+          // use sload from slot (like pointer dereference) to avoid gas
+          assembly {
+            recipient := sload(receiverSlot)
+          }
+          (uint256 feeSent0, uint256 feeSent1) = _transferFees(feePending0, feePending1, recipient);
+          // use sload from slot (like pointer dereference) to avoid gas
+          assembly {
+            sstore(feePendingSlot, 0)
+          }
+          // sent fees return 0 pending and sent fees
+          return (0, 0, feeSent0, feeSent1);
+        }
+      }
+      // didn't either sent fees or increased pending
+      return (0, 0, 0, 0);
+    }
+  }
 
+  function _transferFees(uint256 feePending0, uint256 feePending1, address feesRecipient) private returns (uint256, uint256) {
     uint256 feeSent0;
     uint256 feeSent1;
 
-    if (_blockTimestamp() - lastTimestamp >= Constants.FEE_TRANSFER_FREQUENCY || feePending0 > type(uint104).max || feePending1 > type(uint104).max) {
-      if (feePending0 > 0) {
-        _transfer(token0, feesRecipient, feePending0);
-        deltaR0 = deltaR0 - feePending0.toInt256();
-        feeSent0 = feePending0;
-        feePending0 = 0;
-      }
-      if (feePending1 > 0) {
-        _transfer(token1, feesRecipient, feePending1);
-        deltaR1 = deltaR1 - feePending1.toInt256();
-        feeSent1 = feePending1;
-        feePending1 = 0;
-      }
+    if (feePending0 > 0) {
+      _transfer(token0, feesRecipient, feePending0);
+      feeSent0 = feePending0;
     }
-
-    assembly {
-      sstore(slot, or(or(feePending0, shl(104, feePending1)), shl(208, lastTimestamp)))
+    if (feePending1 > 0) {
+      _transfer(token1, feesRecipient, feePending1);
+      feeSent1 = feePending1;
     }
 
-    return (deltaR0, deltaR1, feeSent0, feeSent1);
+    return (feeSent0, feeSent1);
   }
 
   /// @notice Applies deltas to reserves and pays communityFees
@@ -133,29 +178,57 @@ abstract contract ReservesManager is AlgebraPoolBase {
     uint256 pluginFee1
   ) internal {
     if (communityFee0 > 0 || communityFee1 > 0 || pluginFee0 > 0 || pluginFee1 > 0) {
-      bytes32 slot;
+      bytes32 feePendingSlot;
+      bytes32 feeRecipientSlot;
       uint32 lastTimestamp = lastFeeTransferTimestamp;
-      uint32 currentTimestamp = _blockTimestamp();
-      if (communityFee0 | communityFee1 != 0) {
-        assembly {
-          slot := communityFeePending0.slot
-        }
-        (deltaR0, deltaR1, , ) = updateFeeAmounts(deltaR0, deltaR1, communityFee0, communityFee1, communityVault, slot, lastTimestamp);
+      bool feeSent;
+
+      assembly {
+        feePendingSlot := communityFeePending0.slot
+        feeRecipientSlot := communityVault.slot
+      }
+      // pass feeRecipientSlot to avoid redundant sload of an address
+      (uint104 feePending0, uint104 feePending1, uint256 feeSent0, uint256 feeSent1) = _accrueAndTransferFees(
+        communityFee0,
+        communityFee1,
+        lastTimestamp,
+        feeRecipientSlot,
+        feePendingSlot
+      );
+      if (feeSent0 | feeSent1 != 0) {
+        // sent fees so decrease deltas
+        (deltaR0, deltaR1) = (deltaR0 - feeSent0.toInt256(), deltaR1 - feeSent1.toInt256());
+        feeSent = true;
+      } else {
+        // update pending if we accrued fees
+        if (feePending0 | feePending1 != 0) (communityFeePending0, communityFeePending1) = (feePending0, feePending1);
       }
 
-      if (pluginFee0 | pluginFee1 != 0) {
-        assembly {
-          slot := pluginFeePending0.slot
-        }
-        uint256 pluginFeeSent0;
-        uint256 pluginFeeSent1;
-        (deltaR0, deltaR1, pluginFeeSent0, pluginFeeSent1) = updateFeeAmounts(deltaR0, deltaR1, pluginFee0, pluginFee1, plugin, slot, lastTimestamp);
-        if (pluginFeeSent0 > 0 || pluginFeeSent1 > 0) {
-          if (IAlgebraPlugin(plugin).handlePluginFee(pluginFeeSent0, pluginFeeSent1) != IAlgebraPlugin.handlePluginFee.selector) revert IAlgebraPoolErrors.invalidPluginResponce();  
-        }
+      assembly {
+        feePendingSlot := pluginFeePending0.slot
+        feeRecipientSlot := plugin.slot
+      }
+      // pass feeRecipientSlot to avoid redundant sload of an address
+      (feePending0, feePending1, feeSent0, feeSent1) = _accrueAndTransferFees(
+        pluginFee0,
+        pluginFee1,
+        lastTimestamp,
+        feeRecipientSlot,
+        feePendingSlot
+      );
+      if (feeSent0 | feeSent1 != 0) {
+        // sent fees so decrease deltas
+        (deltaR0, deltaR1) = (deltaR0 - feeSent0.toInt256(), deltaR1 - feeSent1.toInt256());
+        feeSent = true;
+
+        // notify plugin about sent fees
+        IAlgebraPlugin(plugin).handlePluginFee(feeSent0, feeSent1).shouldReturn(IAlgebraPlugin.handlePluginFee.selector);
+      } else {
+        // update pending if we accrued fees
+        if (feePending0 | feePending1 != 0) (pluginFeePending0, pluginFeePending1) = (feePending0, feePending1);
       }
 
-      if (currentTimestamp - lastTimestamp >= Constants.FEE_TRANSFER_FREQUENCY) lastFeeTransferTimestamp = currentTimestamp;
+      if (feeSent) lastFeeTransferTimestamp = _blockTimestamp();
     }
 
     if (deltaR0 | deltaR1 == 0) return;

src/core/contracts/base/SwapCalculation.sol

@@ -60,7 +60,15 @@ abstract contract SwapCalculation is AlgebraPoolBase {
     // load from one storage slot too
     (currentPrice, currentTick, cache.fee, cache.communityFee) = (globalState.price, globalState.tick, globalState.lastFee, globalState.communityFee);
     if (currentPrice == 0) revert notInitialized();
-    if (overrideFee != 0) cache.fee = overrideFee;
+    if (overrideFee != 0) {
+      cache.fee = overrideFee + pluginFee;
+      if (cache.fee >= 1e6) revert incorrectPluginFee();
+    } else {
+      if (pluginFee != 0) {
+        cache.fee += pluginFee;
+        if (cache.fee >= 1e6) revert incorrectPluginFee();
+      }
+    }
 
     if (zeroToOne) {
       if (limitSqrtPrice >= currentPrice || limitSqrtPrice <= TickMath.MIN_SQRT_RATIO) revert invalidLimitSqrtPrice();

src/core/contracts/interfaces/pool/IAlgebraPoolErrors.sol

@@ -28,9 +28,6 @@ interface IAlgebraPoolErrors {
   /// @notice Emitted if plugin fee param greater than fee/override fee
   error incorrectPluginFee();
 
-  /// @notice Emitted if a plugin returns invalid selector after pluginFeeHandle call
-  error invalidPluginResponce();
-
   /// @notice Emitted if the pool received fewer tokens than it should have
   error insufficientInputAmount();
 

src/core/contracts/test/MockPoolPlugin.sol

@@ -110,8 +110,8 @@ contract MockPoolPlugin is IAlgebraPlugin, IAlgebraDynamicFeePlugin {
   ) external override returns (bytes4, uint24) {
     emit BeforeModifyPosition(sender, recipient, bottomTick, topTick, desiredLiquidityDelta, data);
     if (!Plugins.hasFlag(selectorsDisableConfig, Plugins.BEFORE_POSITION_MODIFY_FLAG))
-      return (IAlgebraPlugin.beforeModifyPosition.selector, overrideFee);
-    return (IAlgebraPlugin.defaultPluginConfig.selector, overrideFee);
+      return (IAlgebraPlugin.beforeModifyPosition.selector, pluginFee);
+    return (IAlgebraPlugin.defaultPluginConfig.selector, pluginFee);
   }
 
   /// @notice The hook called after a position is modified

src/core/hardhat.config.ts

@@ -39,7 +39,7 @@ const HIGH_COMPILER_SETTINGS: SolcUserConfig = {
     evmVersion: 'paris',
     optimizer: {
       enabled: true,
-      runs: 500,
+      runs: 0,
     },
     metadata: {
       bytecodeHash: 'none',