Merge pull request #430 from primitivefinance/audit/spearbit-july

Audit/spearbit july

contracts/Portfolio.sol

@@ -112,27 +112,75 @@ contract Portfolio is ERC1155, IPortfolio {
     uint64 private _getLastPoolId;
 
     /**
+     * @notice
+     * Custom mutex designed to prevent injected single-call re-entrancy
+     * during multi-calls.
+     *
      * @dev
-     * Protects against reentrancy and getting to invalid settlement states.
+     * Protects against re-entrancy and getting to invalid settlement states.
      * This lock works in pair with `_postLock` and both should be used on all
      * external non-view functions (except the restricted ones).
      *
+     * Upon entering the mutex, there can be two conditions:
+     * 1. `_locked != 1`. If not in a multicall, i.e. `&& !_currentMulticall`,
+     *                    revert (singleCall -> singleCall re-entrancy).
+     * 2. `_locked > 2`.  Reached here from starting in multicall.
+     *                    This was called from another singleCall that did not have postLock() fire,
+     *                    i.e. `_locked > 2`, revert because its injected re-entrancy:
+     *                    (multiCall -> singleCall -> injected singleCall re-entrancy).
+     *
+     * This enforces that only the originally defined multicall actions are run,
+     * and injected single calls are successfully caught and reverted by the mutex.
+     * These injected re-entrancy guards are caught because the `postLock()` never gets called,
+     * which decrements the `_locked` variable.
+     * Multicalls go through because they don't fire the `postLock()` until after the calls ran.
+     *
+     * @custom:example Scenarios:
+     * 1. reenter during multicall's action execution
+     * multicall -> _currentMulticall = true
+     *   preLock() -> _locked++ = 2
+     *     singleCall()
+     *       preLock() -> _locked++ = 3
+     *       reenter during current execution (injected) -> postLock() does not decrement _locked.
+     *         singeCall()
+     *           preLock(): (_locked != 1 && (false || true)) == true, revert
+     *   _currentMulticall = false;
+     *   settlement()
+     *   postLock()
+     *
+     * 2. reenter during multicall's settlement
+     * multicall -> _currentMulticall = true
+     *   preLock() -> _locked++ = 2
+     *     singleCall
+     *       preLock(): -> _locked++ == 3
+     *       postLock(): -> _locked-- == 2
+     *   _currentMulticall = false;
+     *   settlement() -> _locked == 2
+     *     reenter
+     *       singeCall()
+     *         preLock(): (_locked != 1 && (true || false)) == true, revert
+     *   ...if continued (somehow gets passed the revert)
+     *       mutliCall()
+     *         passes multicall re-entrancy guard because not in multicall
+     *         preLock(): (_locked != 1 && (true || false)) == true, revert
+     *   ... settlement finishes
+     *   postLock(): -> _locked-- == 1
+     *
      * note
      * Private functions are used instead of modifiers to reduce the size
      * of the bytecode.
      */
     function _preLock() private {
-        // Reverts if the lock was already set and the current call is not a multicall.
-        if (_locked != 1 && !_currentMulticall) {
+        if (_locked != 1 && (!_currentMulticall || _locked > 2)) {
             revert Portfolio_InvalidReentrancy();
         }
 
-        _locked = 2;
+        _locked++;
     }
 
     /// @dev Second part of the reentracy guard (see `_preLock`).
     function _postLock() private {
-        _locked = 1;
+        _locked--;
 
         // Reverts if the account system was not settled after a normal call.
         if (!__account__.settled && !_currentMulticall) {
@@ -681,9 +729,16 @@ contract Portfolio is ERC1155, IPortfolio {
         // Increment the pool nonce.
         uint32 poolNonce = ++getPoolNonce[pairNonce];
 
+        // Zero address strtaegy is a magic value to use the default strategy.
+        strategy = strategy == address(0) ? DEFAULT_STRATEGY : strategy;
+
         // Compute the poolId, which is a packed 64-bit integer.
-        bool hasController = controller != address(0);
-        poolId = AssemblyLib.encodePoolId(pairNonce, hasController, poolNonce);
+        poolId = PoolIdLib.encode(
+            strategy != DEFAULT_STRATEGY, // Flips the "altered" flag in the upper 4 bits: "0x10..."
+            controller != address(0), // Flips the "controlled" flag in the lower 4 bits:  "0x01..."
+            pairNonce,
+            poolNonce
+        );
 
         // Instantiate the pool.
         pools[poolId].createPool({
@@ -692,12 +747,13 @@ contract Portfolio is ERC1155, IPortfolio {
             feeBasisPoints: feeBasisPoints,
             priorityFeeBasisPoints: priorityFeeBasisPoints,
             controller: controller,
-            strategy: strategy == address(0) ? DEFAULT_STRATEGY : strategy
+            strategy: strategy
         });
 
         // Store the last created poolId for the multicall, to make sure the user is not frontrun.
         _getLastPoolId = poolId;
 
+        // This call also prevents accidently creating a pool with an invalid strategy target address.
         IStrategy(getStrategy(poolId)).afterCreate(poolId, strategyArgs);
 
         emit CreatePool(

contracts/interfaces/IPortfolio.sol

@@ -571,7 +571,26 @@ interface IPortfolioActions is IPortfolioRegistryActions {
      * @notice
      * Instantiate a pool at a desired price with custom fees and arguments.
      *
+     * @dev
+     * WARNING: Pools can be created with __malicious__ `strategy` contracts.
+     * The `strategy` contracts have the power to create custom `swap` validation logic,
+     * and also can prevent the pool from being allocated to or deallocated from.
+     * Pools are created with a default strategy that uses standard mathematics based
+     * swap validation logic. Pools can never have their strategy changed after creation.
+     *
+     * Pools can have a `controller` address which has limited power over the pool.
+     * Pool controllers have the power to adjust the `feeBasisPoints` and `priorityFeeBasisPoints`.
+     * These can only be adjusted within reasonable bounds: the MIN and MAX fee bounds.
+     *
      * @param pairId Nonce of the target pair. A `0` is a magic variable to use the state variable `getPairNonce` instead.
+     * @param reserveXPerWad Initial virtual reserve for the `asset` token, per WAD units of liquidity and in WAD units.
+     * @param reserveYPerWad Initial virtual reserve for the `quote` token, per WAD units of liquidity and in WAD units.
+     * @param feeBasisPoints Fee charged on swaps, denominated in basis points (1 = 0.01%).
+     * @param priorityFeeBasisPoints Fee charged on swaps if the swap is initiated by the `controller`, denominated in basis points (1 = 0.01%).
+     * @param controller Address that can adjust the pool's fee and priority fee within reasonable bounds.
+     * @param strategy Address of the external strategy contract that handles swap validation.
+     * @param strategyArgs Abi-encoded arguments that are passed to the `strategy` contract via the `afterCreate` hook.
+     * @return poolId Packed encoded custom identifier for the pool. See `PoolLib.sol` for more details.
      */
     function createPool(
         uint24 pairId,

contracts/libraries/AssemblyLib.sol

@@ -138,34 +138,6 @@ library AssemblyLib {
         outputDec = (amountWad - 1) / factor + 1; // ((a-1) / b) + 1
     }
 
-    /**
-     * @dev
-     * Returns an encoded pool id given specific pool parameters.
-     * The encoding is simply packing the different parameters together.
-     *
-     * @custom:example
-     * ```
-     * uint64 poolId = encodePoolId(7, true, 42);
-     * assertEq(poolId, 0x000007010000002a);
-     * uint64 poolIdEncoded = abi.encodePacked(uint24(pairId), uint8(isMutable ? 1 : 0), uint32(poolNonce));
-     * assertEq(poolIdEncoded, poolId);
-     * ```
-     *
-     * @param pairId Id of the pair of asset / quote tokens
-     * @param isMutable True if the pool is mutable
-     * @param poolNonce Current pool nonce of the Portfolio contract
-     * @return poolId Corresponding encoded pool id
-     */
-    function encodePoolId(
-        uint24 pairId,
-        bool isMutable,
-        uint32 poolNonce
-    ) internal pure returns (uint64 poolId) {
-        assembly {
-            poolId := or(or(shl(40, pairId), shl(32, isMutable)), poolNonce)
-        }
-    }
-
     /// @dev Converts basis points (1 = 0.01%) to percentages in WAD units (1E18 = 100%).
     function bpsToPercentWad(uint256 bps)
         internal
@@ -183,4 +155,22 @@ library AssemblyLib {
 
         y = uint16(x);
     }
+
+    /// @dev Packs 4 bits into the upper and lower sections of a byte (8-bits).
+    function pack(
+        bytes1 upper,
+        bytes1 lower
+    ) internal pure returns (bytes1 data) {
+        data = upper << 4 | (lower & 0x0F);
+    }
+
+    /// @dev Separates the upper 4 and lower 4 bits of a byte (8-bits).
+    function separate(bytes1 data)
+        internal
+        pure
+        returns (bytes1 upper, bytes1 lower)
+    {
+        upper = data >> 4;
+        lower = data & 0x0f;
+    }
 }

contracts/libraries/BisectionLib.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.4;
 /// @dev Thrown when the lower bound is greater than the upper bound.
 error BisectionLib_InvalidBounds(uint256 lower, uint256 upper);
 /// @dev Thrown when the result of the function `fx` for each input, `upper` and `lower`, is the same sign.
-error BisectionLib_RootOutsideBounds(uint256 lower, uint256 upper);
+error BisectionLib_RootOutsideBounds(int256 lowerResult, int256 upperResult);
 
 /**
  * @notice
@@ -38,7 +38,7 @@ function bisection(
     int256 lowerOutput = fx(args, lower);
     int256 upperOutput = fx(args, upper);
     if (lowerOutput * upperOutput > 0) {
-        revert BisectionLib_RootOutsideBounds(lower, upper);
+        revert BisectionLib_RootOutsideBounds(lowerOutput, upperOutput);
     }
 
     // Distance is optimized to equal `epsilon`.

contracts/libraries/PoolLib.sol

@@ -6,6 +6,7 @@ import "solmate/utils/SafeCastLib.sol";
 import "./AssemblyLib.sol";
 
 using AssemblyLib for uint256;
+using AssemblyLib for bytes1;
 using FixedPointMathLib for uint256;
 using FixedPointMathLib for uint128;
 using FixedPointMathLib for int256;
@@ -17,20 +18,104 @@ using PoolIdLib for PoolId global;
 
 /// @dev Helper methods to decode the data encoded in a pool id.
 library PoolIdLib {
-    /// @dev Pair id is encoded in the first 24-bits.
+    /**
+     * @notice
+     * Packs key pool information into a 64-bit pool id.
+     *
+     * @dev
+     * Pools with a leading upper flag set, i.e. `0x10...`, MUST be approached with caution, as this
+     * indicates that the pool has a non-default strategy contract. The strategy address
+     * has power over a pool's key swap validation logic and its liquidity logic.
+     *
+     * Pools with a leading lower flag set, i.e. `0x01...`, should be approached with some caution,
+     * as this indicates that the pool has a non-zero address controller. The controller
+     * has power to change the swap and priority swap fees of the pool, within reasonable bounds.
+     *
+     * @custom:example
+     * ```
+     * bool altered = true;
+     * bool controlled = false;
+     * uint64 poolId = encode(altered, controlled, 7,  42);
+     * assertEq(poolId, 0x100000070000002a);
+     * bytes1 packedBools = AssemblyLib.pack(bytes1(uint8(altered ? 1 : 0)), bytes1(uint8(controlled ? 1 : 0)));
+     * uint64 poolIdEncoded = abi.encodePacked(packedBools, uint24(pairId), uint32(poolNonce));
+     * assertEq(poolIdEncoded, poolId);
+     * ```
+     *
+     * @param altered Whether the pool has the non-default strategy address.
+     * @param controlled Whether the pool has a non-zero address controller.
+     * @param pairId Id of the pair of asset / quote tokens.
+     * @param poolNonce Current pool nonce of the created pools for the `pairId`.
+     * @return poolId Packs the above arguments into a 64-bit pool identifier.
+     */
+    function encode(
+        bool altered,
+        bool controlled,
+        uint24 pairId,
+        uint32 poolNonce
+    ) internal pure returns (uint64 poolId) {
+        // Packs key information about a pool into a single byte.
+        // If the pool has a controller and if it has a non-default strategy are pieces of information
+        // that can be read directly from the pool id.
+        // Pool ids with a leading 0x11 byte should be approached with caution,
+        // while the pool ids with a leading 0x00 byte are using default parameters.
+        uint8 packed = uint8(
+            AssemblyLib.pack(
+                bytes1(uint8(altered ? 1 : 0)),
+                bytes1(uint8(controlled ? 1 : 0))
+            )
+        );
+
+        assembly {
+            // First, shifts the single byte packed flags all the way to the
+            // first 8 bits of the 64-bit pool id (64 - 8 = 56).
+            // Second, shifts the pairId to the 24 bits after the flags, (56 - 24 = 32).
+            // Third, places the pool nonce in the last 32 bits of the pool id (32 - 32 = 0).
+            // This algorithm packs the 64-bit pool id with key information, in the following layout:
+            // 0x       00             000000000000 0000000000000000
+            //          ^^             ^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
+            // (altered, controlled)      pairId       poolNonce
+            poolId := or(or(shl(56, packed), shl(32, pairId)), poolNonce)
+        }
+    }
+
+    /// @dev Helper function to decode a pool id into its key information components.
+    function decode(uint64 poolId)
+        internal
+        pure
+        returns (bool controlled, bool altered, uint24 pairId, uint32 nonce)
+    {
+        PoolId id = PoolId.wrap(poolId);
+        controlled = id.controlled();
+        altered = id.altered();
+        pairId = id.pairId();
+        nonce = id.nonce();
+    }
+
+    /// @dev Pair id is the 24-bits to the left of the 32-bit pool nonce at the end.
     function pairId(PoolId poolId) internal pure returns (uint24 id) {
-        id = uint24(PoolId.unwrap(poolId) >> 40);
+        id = uint24(PoolId.unwrap(poolId) >> 32);
     }
 
-    /// @dev Controlled boolean is between the first 24-bits and last 32-bits.
+    /// @dev Controlled flag true if the pool has a controller. Lower 4 bits of the first byte.
     function controlled(PoolId poolId) internal pure returns (bool) {
-        return uint8(PoolId.unwrap(poolId) >> 32) == 1;
+        (, bytes1 controlled) =
+            bytes1(uint8(PoolId.unwrap(poolId) >> 56)).separate();
+        return uint8(controlled) == 1;
     }
 
     /// @dev Nonce is encoded in the last 32-bits.
     function nonce(PoolId poolId) internal pure returns (uint32 nonce) {
+        // Uses an unsafe cast to uint32 to truncate the pool id to its last 32 bits.
         nonce = uint32(PoolId.unwrap(poolId));
     }
+
+    /// @dev Altered flag is true if the pool does __not__ use the default strategy. Upper 4 bits of the first byte.
+    function altered(PoolId poolId) internal pure returns (bool) {
+        (bytes1 altered,) =
+            bytes1(uint8(PoolId.unwrap(poolId) >> 56)).separate();
+        return uint8(altered) == 1;
+    }
 }
 
 using {
@@ -191,6 +276,10 @@ function adjustReserves(
         self.virtualX -= deltaOutWad.safeCastTo128();
         self.virtualY += deltaInWad.safeCastTo128();
     }
+
+    // Verify the reserves are not zero. Prevents one side of the pool from ever being 0.
+    if (self.virtualX == 0) revert PoolLib_InvalidReserveX();
+    if (self.virtualY == 0) revert PoolLib_InvalidReserveY();
 }
 
 // ----------------- //
@@ -221,6 +310,8 @@ function isActive(PortfolioPool memory self) pure returns (bool) {
  * Must be used offchain, or else the pool's reserves can be manipulated to
  * take advantage of this function's reliance on the reserves.
  * This function can be used in conjuction with `getPoolLiquidityDeltas` to compute the maximum `allocate()` for a user.
+ * Prevents zero amount allocates by returning a 0 liquidity amount if one of the deltas is 0.
+ * Attempting to allocate 0 liquidity will revert.
  *
  * @param deltaAsset Desired amount of `asset` to allocate, denominated in WAD.
  * @param deltaQuote Desired amount of `quote` to allocate, denominated in WAD.
@@ -246,8 +337,6 @@ function getPoolMaxLiquidity(
     if(self.virtualY != 0) liquidity1 = deltaQuote.mulDivDown(totalLiquidity, self.virtualY);  // forgefmt: disable-line
     // Use the smaller of the two liquidity amounts, which should be used to compute the liquidity deltas.
     deltaLiquidity = AssemblyLib.min(liquidity0, liquidity1).safeCastTo128(); // forgefmt: disable-line
-    // When one of the reserves is 0, then liquidity can be minted from only one token being allocated.
-    if(deltaLiquidity == 0) AssemblyLib.max(liquidity0, liquidity1).safeCastTo128(); // forgefmt: disable-line
 }
 
 /**

contracts/libraries/SwapLib.sol

@@ -14,7 +14,6 @@ using {
 using AssemblyLib for uint256;
 using FixedPointMathLib for uint128;
 
-error SwapLib_FeeTooHigh();
 error SwapLib_ProtocolFeeTooHigh();
 error SwapLib_OutputExceedsReserves();
 error SwapLib_ZeroXAdjustment();
@@ -116,7 +115,7 @@ function computeAdjustedSwapReserves(
     // Input amount is added to the reserves for the swap.
     adjustedInputReserveWad += self.input;
     // Fee amount is reinvested into the pool, but it's not considered in the invariant check, so we subtract it.
-    if (feeAmountUnit > adjustedInputReserveWad) revert SwapLib_FeeTooHigh();
+    // Assumes the fee % is always less than 100%, therefore this cannot underflow.
     adjustedInputReserveWad -= feeAmountUnit;
     // Protocol fee is subtracted, even though it's included in the fee, because protocol fees
     // do not get added to the pool's reserves.