fix: cantina audit issues (#8)

* refund ETH dust in supplier create pool (#2) * remove pool finalize callback * update supplier finalize to use amounts returned * check finalizer not zero in supplier create pool params * index supplier in factory create pool event * add finalized boolean to pool swap event * fix round up on pool mint with range amounts * fix pool swap clamp to limit amount in * liquidity receiver update univ3 reserves with returned amounts * refund address in liquidity receiver params * check zero addresses in liquidity receiver check params * supplier immutable in liquidity receiver deployer * liquidity receiver free unspent reserves after lock duration * remove todos in liquidity receiver * test router exact input single * test router exact output single * test router exact output single reverts outside tick range * test router exact input single clamps outside tick range * remove checkDeadline from supplier
MarginalProtocol · Sep 10, 2024 · ebd8772 · ebd8772
1 parent 1137a3b
commit ebd8772
Show file tree

Hide file tree

Showing 30 changed files with 2,479 additions and 294 deletions.
diff --git a/contracts/MarginalV1LBFactory.sol b/contracts/MarginalV1LBFactory.sol
@@ -33,7 +33,7 @@ contract MarginalV1LBFactory is IMarginalV1LBFactory {
         address indexed token1,
         int24 tickLower,
         int24 tickUpper,
-        address supplier,
+        address indexed supplier,
         uint256 blockTimestampInitialize,
         address pool
     );

diff --git a/contracts/MarginalV1LBPool.sol b/contracts/MarginalV1LBPool.sol
@@ -18,7 +18,6 @@ import {IMarginalV1SwapCallback} from "@marginal/v1-core/contracts/interfaces/ca
 
 import {RangeMath} from "./libraries/RangeMath.sol";
 
-import {IMarginalV1LBFinalizeCallback} from "./interfaces/callback/IMarginalV1LBFinalizeCallback.sol";
 import {IMarginalV1LBFactory} from "./interfaces/IMarginalV1LBFactory.sol";
 import {IMarginalV1LBPool} from "./interfaces/IMarginalV1LBPool.sol";
 
@@ -82,7 +81,8 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
         int256 amount1,
         uint160 sqrtPriceX96,
         uint128 liquidity,
-        int24 tick
+        int24 tick,
+        bool finalized
     );
     event Mint(
         address sender,
@@ -184,7 +184,7 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
 
     /// @inheritdoc IMarginalV1LBPool
     function finalize(
-        bytes calldata data
+        address recipient
     )
         external
         returns (
@@ -205,17 +205,10 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
         // burn liquidity to supplier
         (liquidityDelta, amount0, amount1, fees0, fees1) = burn(
             address(this),
-            msg.sender,
+            recipient,
             _totalSupply
         );
-
-        // notify supplier of funds transferred on burn
         sqrtPriceX96 = state.sqrtPriceX96;
-        IMarginalV1LBFinalizeCallback(msg.sender).marginalV1LBFinalizeCallback(
-            amount0,
-            amount1,
-            data
-        );
 
         emit Finalize(liquidityDelta, sqrtPriceX96, state.tick);
     }
@@ -283,16 +276,19 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
         ) revert SqrtPriceX96ExceedsLimit();
 
         // clamp if exceeds lower or upper range limits
-        // @dev no need to revert on exact input as trader pays more than necessary
+        bool clamped;
         if (
             !exactInput &&
             (sqrtPriceX96Next < sqrtPriceLowerX96 ||
                 sqrtPriceX96Next > sqrtPriceUpperX96)
         ) revert RangeMath.InvalidSqrtPriceX96();
-        else if (sqrtPriceX96Next < sqrtPriceLowerX96)
+        else if (sqrtPriceX96Next < sqrtPriceLowerX96) {
             sqrtPriceX96Next = sqrtPriceLowerX96;
-        else if (sqrtPriceX96Next > sqrtPriceUpperX96)
+            clamped = true;
+        } else if (sqrtPriceX96Next > sqrtPriceUpperX96) {
             sqrtPriceX96Next = sqrtPriceUpperX96;
+            clamped = true;
+        }
 
         // amounts without fees
         (amount0, amount1) = SwapMath.swapAmounts(
@@ -304,7 +300,7 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
         // optimistic amount out with callback for amount in
         if (!zeroForOne) {
             amount0 = !exactInput ? amountSpecified : amount0; // in case of rounding issues
-            amount1 = exactInput ? amountSpecified : amount1;
+            amount1 = exactInput && !clamped ? amountSpecified : amount1;
 
             if (amount0 < 0)
                 TransferHelper.safeTransfer(
@@ -326,7 +322,7 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
             _state.tick = TickMath.getTickAtSqrtRatio(sqrtPriceX96Next);
         } else {
             amount1 = !exactInput ? amountSpecified : amount1; // in case of rounding issues
-            amount0 = exactInput ? amountSpecified : amount0;
+            amount0 = exactInput && !clamped ? amountSpecified : amount0;
 
             if (amount1 < 0)
                 TransferHelper.safeTransfer(
@@ -361,7 +357,8 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
             amount1,
             _state.sqrtPriceX96,
             _state.liquidity,
-            _state.tick
+            _state.tick,
+            _state.finalized
         );
     }
 
@@ -386,8 +383,8 @@ contract MarginalV1LBPool is IMarginalV1LBPool, ERC20 {
             sqrtPriceLowerX96,
             sqrtPriceUpperX96
         );
-        amount0 += 1; // rough round up on amounts in when add liquidity
-        amount1 += 1;
+        if (_state.sqrtPriceX96 != sqrtPriceUpperX96) amount0 += 1; // rough round up on amounts in when add liquidity
+        if (_state.sqrtPriceX96 != sqrtPriceLowerX96) amount1 += 1;
 
         // total liquidity is available liquidity if all locked liquidity was returned to pool
         uint128 totalLiquidityAfter = _state.liquidity + liquidityDelta;

diff --git a/contracts/MarginalV1LBSupplier.sol b/contracts/MarginalV1LBSupplier.sol
@@ -3,7 +3,6 @@ pragma solidity =0.8.15;
 
 import {TickMath} from "@uniswap/v3-core/contracts/libraries/TickMath.sol";
 import {LiquidityAmounts} from "@uniswap/v3-periphery/contracts/libraries/LiquidityAmounts.sol";
-import {PeripheryValidation} from "@uniswap/v3-periphery/contracts/base/PeripheryValidation.sol";
 import {Multicall} from "@uniswap/v3-periphery/contracts/base/Multicall.sol";
 
 import {IMarginalV1MintCallback} from "@marginal/v1-core/contracts/interfaces/callback/IMarginalV1MintCallback.sol";
@@ -14,7 +13,6 @@ import {RangeMath} from "./libraries/RangeMath.sol";
 import {PeripheryImmutableState} from "./base/PeripheryImmutableState.sol";
 import {PeripheryPayments} from "./base/PeripheryPayments.sol";
 
-import {IMarginalV1LBFinalizeCallback} from "./interfaces/callback/IMarginalV1LBFinalizeCallback.sol";
 import {IMarginalV1LBReceiverDeployer} from "./interfaces/receiver/IMarginalV1LBReceiverDeployer.sol";
 
 import {IMarginalV1LBReceiver} from "./interfaces/receiver/IMarginalV1LBReceiver.sol";
@@ -25,18 +23,10 @@ import {IMarginalV1LBSupplier} from "./interfaces/IMarginalV1LBSupplier.sol";
 contract MarginalV1LBSupplier is
     IMarginalV1LBSupplier,
     IMarginalV1MintCallback,
-    IMarginalV1LBFinalizeCallback,
     PeripheryImmutableState,
     PeripheryPayments,
-    PeripheryValidation,
     Multicall
 {
-    uint256 private constant DEFAULT_BALANCE_CACHED = type(uint256).max;
-
-    /// @dev Transient storage variables used for computing amounts received on finalize callback
-    uint256 private balance0Cached = DEFAULT_BALANCE_CACHED;
-    uint256 private balance1Cached = DEFAULT_BALANCE_CACHED;
-
     /// @inheritdoc IMarginalV1LBSupplier
     mapping(address => address) public receivers;
 
@@ -45,6 +35,7 @@ contract MarginalV1LBSupplier is
 
     error Unauthorized();
     error InvalidPool();
+    error InvalidFinalizer();
     error InvalidReceiver();
     error Amount0LessThanMin();
     error Amount1LessThanMin();
@@ -87,7 +78,6 @@ contract MarginalV1LBSupplier is
     )
         external
         payable
-        checkDeadline(params.deadline)
         returns (
             address pool,
             address receiver,
@@ -111,6 +101,7 @@ contract MarginalV1LBSupplier is
             address(this),
             block.timestamp // use current block timestamp
         );
+        if (params.finalizer == address(0)) revert InvalidFinalizer();
         finalizers[pool] = params.finalizer;
 
         // deploy the receiver after creating liquidity bootstrapping pool
@@ -161,6 +152,9 @@ contract MarginalV1LBSupplier is
 
         if (amount0 < params.amount0Min) revert Amount0LessThanMin();
         if (amount1 < params.amount1Min) revert Amount1LessThanMin();
+
+        // refund any excess ETH to sender at end of function to avoid re-entrancy with fallback
+        refundETH();
     }
 
     struct MintCallbackData {
@@ -188,7 +182,6 @@ contract MarginalV1LBSupplier is
         FinalizeParams calldata params
     )
         external
-        checkDeadline(params.deadline)
         returns (
             uint128 liquidityDelta,
             uint160 sqrtPriceX96,
@@ -215,73 +208,17 @@ contract MarginalV1LBSupplier is
         (, , , , , , , bool finalized) = IMarginalV1LBPool(pool).state();
         if (!finalized && msg.sender != finalizers[pool]) revert Unauthorized();
 
-        // cache balances for check on finalize callback on amounts received
-        balance0Cached = balance(poolKey.token0);
-        balance1Cached = balance(poolKey.token1);
-
         (
             liquidityDelta,
             sqrtPriceX96,
             amount0,
             amount1,
             fees0,
             fees1
-        ) = IMarginalV1LBPool(pool).finalize(
-            abi.encode(
-                FinalizeCallbackData({poolKey: poolKey, receiver: receiver})
-            )
-        );
-    }
-
-    struct FinalizeCallbackData {
-        PoolAddress.PoolKey poolKey;
-        address receiver;
-    }
-
-    /// @inheritdoc IMarginalV1LBFinalizeCallback
-    function marginalV1LBFinalizeCallback(
-        uint256 amount0Transferred,
-        uint256 amount1Transferred,
-        bytes calldata data
-    ) external {
-        FinalizeCallbackData memory decoded = abi.decode(
-            data,
-            (FinalizeCallbackData)
-        );
-        CallbackValidation.verifyCallback(factory, decoded.poolKey);
-
-        // only support tokens with standard ERC20 transfer
-        if (
-            balance0Cached + amount0Transferred >
-            balance(decoded.poolKey.token0)
-        ) revert Amount0LessThanMin();
-        if (
-            balance1Cached + amount1Transferred >
-            balance(decoded.poolKey.token1)
-        ) revert Amount1LessThanMin();
-
-        if (amount0Transferred > 0)
-            pay(
-                decoded.poolKey.token0,
-                address(this),
-                decoded.receiver,
-                amount0Transferred
-            );
-        if (amount1Transferred > 0)
-            pay(
-                decoded.poolKey.token1,
-                address(this),
-                decoded.receiver,
-                amount1Transferred
-            );
-
-        IMarginalV1LBReceiver(decoded.receiver).notifyRewardAmounts(
-            amount0Transferred,
-            amount1Transferred
-        );
+        ) = IMarginalV1LBPool(pool).finalize(receiver);
 
-        // reset balances cached
-        balance0Cached = DEFAULT_BALANCE_CACHED;
-        balance1Cached = DEFAULT_BALANCE_CACHED;
+        // notify receiver of forwarded funds
+        // @dev only supports tokens with standard ERC20 transfer
+        IMarginalV1LBReceiver(receiver).notifyRewardAmounts(amount0, amount1);
     }
 }
diff --git a/contracts/interfaces/IMarginalV1LBPool.sol b/contracts/interfaces/IMarginalV1LBPool.sol
@@ -88,16 +88,15 @@ interface IMarginalV1LBPool {
 
     /// @notice Finalizes the liquidity bootstrapping removing liquidity from the pool
     /// @dev Can be called if pool sqrt price has reached final tick or if supplier manually exists after minimum bootstrapping duration.
-    /// The `recipient_` of the pool receives a callback in the form of IMarginalV1LBFinalizeCallback#marginalV1LBFinalizeCallback.
-    /// @param data Any data to be passed through to the finalize callback
+    /// @param recipient The address to receive the output of the liquidity removed from the pool
     /// @return liquidityDelta The liquidity removed from the pool
     /// @return sqrtPriceX96 The final price of the pool as a sqrt(token1/token0) Q64.96 value
     /// @return amount0 The amount of token0 removed from pool reserves less protocol fees
     /// @return amount1 The amount of token1 removed from pool reserves less protocol fees
     /// @return fees0 The amount of token0 taken as protocol fees from pool reserves
     /// @return fees1 The amount of token1 taken as protocol fees from pool reserves
     function finalize(
-        bytes calldata data
+        address recipient
     )
         external
         returns (

diff --git a/contracts/interfaces/IMarginalV1LBSupplier.sol b/contracts/interfaces/IMarginalV1LBSupplier.sol
@@ -1,9 +1,11 @@
 // SPDX-License-Identifier: AGPL-3.0
 pragma solidity ^0.8.0;
 
+import {IPeripheryImmutableState} from "./IPeripheryImmutableState.sol";
+
 /// @title The interface for a Marginal v1 liquidity boostrapping pool supplier
 /// @notice Creates and initializes Marginal v1 liquidity bootstrapping pool, supplier necessary liquidity for LBP
-interface IMarginalV1LBSupplier {
+interface IMarginalV1LBSupplier is IPeripheryImmutableState {
     /// @notice Returns the address of receiver of transferred funds for a liquidity bootstrapping pool
     /// @param pool The liquidity bootstrapping pool
     /// @return The address of the receiver of the raised funds from the liquidity bootstrapping pool
@@ -26,7 +28,6 @@ interface IMarginalV1LBSupplier {
         address receiverDeployer;
         bytes receiverData;
         address finalizer; // can early exit from pool after min duration
-        uint256 deadline;
     }
 
     /// @notice Creates a new liquidity boostrapping pool then initializes
@@ -56,7 +57,6 @@ interface IMarginalV1LBSupplier {
         int24 tickLower;
         int24 tickUpper;
         uint256 blockTimestampInitialize;
-        uint256 deadline;
     }
 
     /// @notice Finalizes an existing liquidity bootstrapping pool, then forwards received funds to recipient stored on initialization

diff --git a/contracts/interfaces/callback/IMarginalV1LBFinalizeCallback.sol b/contracts/interfaces/callback/IMarginalV1LBFinalizeCallback.sol
diff --git a/contracts/interfaces/receiver/liquidity/IMarginalV1LBLiquidityReceiver.sol b/contracts/interfaces/receiver/liquidity/IMarginalV1LBLiquidityReceiver.sol
@@ -22,6 +22,10 @@ interface IMarginalV1LBLiquidityReceiver is IMarginalV1LBReceiver {
     /// @return The amount of token1 left
     function reserve1() external view returns (uint256);
 
+    /// @notice Returns the block timestamp at which receiver was notified
+    /// @return The block timestamp when receiver was notified
+    function blockTimestampNotified() external view returns (uint96);
+
     /// @notice Returns the receiver params set on deployment to use when creating Uniswap v3 and Marginal v1 pools
     /// @return treasuryAddress The address to send treasury ratio funds to
     /// @return treasuryRatio The fraction of lbp funds to send to treasury in units of hundredths of 1 bip
@@ -30,6 +34,7 @@ interface IMarginalV1LBLiquidityReceiver is IMarginalV1LBReceiver {
     /// @return marginalV1Maintenance The minimum maintenance requirement of Marginal v1 pool to add liquidity to
     /// @return lockOwner The address that can unlock liquidity after lock passes from this contract
     /// @return lockDuration The number of seconds after which can unlock liquidity receipt tokens from this contract
+    /// @return refundAddress The address to refund unspent funds to
     function receiverParams()
         external
         view
@@ -40,7 +45,8 @@ interface IMarginalV1LBLiquidityReceiver is IMarginalV1LBReceiver {
             uint24 uniswapV3Fee,
             uint24 marginalV1Maintenance,
             address lockOwner,
-            uint96 lockDuration
+            uint96 lockDuration,
+            address refundAddress
         );
 
     /// @notice Returns the pool information for the created and initialized Uniswap v3 pool
@@ -116,4 +122,8 @@ interface IMarginalV1LBLiquidityReceiver is IMarginalV1LBReceiver {
     /// @dev Reverts if `msg.sender` is not lock owner or if not enough time has passed since Marginal v1 pool liquidity minted
     /// @param recipient The address of the recipient of the unlocked Marginal v1 liquidity shares
     function freeMarginalV1(address recipient) external;
+
+    /// @notice Frees reserve balances left in receiver if enough time has passed
+    /// @dev Reverts if `msg.sender` is not lock owner or if not enough time has passed since receiver notified
+    function freeReserves() external;
 }
diff --git a/contracts/interfaces/receiver/liquidity/IMarginalV1LBLiquidityReceiverDeployer.sol b/contracts/interfaces/receiver/liquidity/IMarginalV1LBLiquidityReceiverDeployer.sol
@@ -8,6 +8,14 @@ import {IMarginalV1LBReceiverDeployer} from "../IMarginalV1LBReceiverDeployer.so
 interface IMarginalV1LBLiquidityReceiverDeployer is
     IMarginalV1LBReceiverDeployer
 {
+    /// @notice Returns the address of the Marginal v1 liquidity bootstrapping pool supplier
+    /// @return The address of the supplier
+    function supplier() external view returns (address);
+
+    /// @notice Returns the address of the Marginal v1 liquidity bootstrapping pool factory
+    /// @return The address of the factory
+    function factory() external view returns (address);
+
     /// @notice Returns the address of the Uniswap v3 nonfungible position manager
     /// @return The address of the Uniswap v3 NFT manager
     function uniswapV3NonfungiblePositionManager()