Merge tag 'audit_ourovoros_oct23' into eip1153_vip

.forge-snapshots/settler_basic_curve_USDT-WETH.snap

@@ -1 +1 @@
-441124
+365506

.github/workflows/integration.yml

@@ -18,7 +18,7 @@ jobs:
           version: nightly
 
       - name: Install dependencies
-        run: forge install
+        run: git submodule update --recursive --init
 
       - name: Check contract sizes
         run: forge build --sizes

.github/workflows/test.yml

@@ -18,7 +18,7 @@ jobs:
           version: nightly
 
       - name: Install dependencies
-        run: forge install
+        run: git submodule update --recursive --init
 
       - name: Check contract sizes
         run: forge build --sizes

.gitmodules

@@ -1,7 +1,6 @@
 [submodule "lib/forge-std"]
 	path = lib/forge-std
 	url = https://github.com/foundry-rs/forge-std
-	branch = v1.5.1
 [submodule "lib/permit2"]
 	path = lib/permit2
 	url = https://github.com/Uniswap/permit2

remappings.txt

@@ -2,3 +2,4 @@ solmate/=lib/solmate
 permit2/=lib/permit2
 forge-std/=lib/forge-std/src
 forge-gas-snapshot/=lib/forge-gas-snapshot/src
+ds-test/=lib/forge-std/lib/ds-test/src

src/ISettlerActions.sol

@@ -5,13 +5,15 @@ import {ISignatureTransfer} from "permit2/src/interfaces/ISignatureTransfer.sol"
 import {IZeroEx} from "./core/ZeroEx.sol";
 
 interface ISettlerActions {
-    // TODO: PERMIT2_TRANSFER_FROM and METATXN_PERMIT2_TRANSFER_FROM need custody optimization
-
     /// @dev Transfer funds from msg.sender Permit2.
-    function PERMIT2_TRANSFER_FROM(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory sig) external;
+    function PERMIT2_TRANSFER_FROM(
+        address recipient,
+        ISignatureTransfer.PermitTransferFrom memory permit,
+        bytes memory sig
+    ) external;
 
     /// @dev Transfer funds from metatransaction requestor into the Settler contract using Permit2. Only for use in `Settler.executeMetaTxn` where the signature is provided as calldata
-    function METATXN_PERMIT2_TRANSFER_FROM(ISignatureTransfer.PermitTransferFrom memory) external;
+    function METATXN_PERMIT2_TRANSFER_FROM(address recipient, ISignatureTransfer.PermitTransferFrom memory) external;
 
     /// @dev Settle an OtcOrder between maker and taker transfering funds directly between the parties
     // Post-req: Payout if recipient != taker
@@ -31,16 +33,15 @@ interface ISettlerActions {
         ISignatureTransfer.PermitTransferFrom memory takerPermit
     ) external;
 
-    // TODO: SETTLER_OTC_SELF_FUNDED needs custody optimization
-
     /// @dev Settle an OtcOrder between Maker and Settler. Transfering funds from the Settler contract to maker.
     /// Retaining funds in the settler contract.
     // Pre-req: Funded
     // Post-req: Payout
     function SETTLER_OTC_SELF_FUNDED(
+        address recipient,
         ISignatureTransfer.PermitTransferFrom memory permit,
         address maker,
-        bytes memory sig,
+        bytes memory makerSig,
         address takerToken,
         uint256 maxTakerAmount
     ) external;
@@ -71,23 +72,9 @@ interface ISettlerActions {
     ) external;
 
     /// @dev Trades against UniswapV2 using the contracts balance for funding
-    function UNISWAPV2_SWAP(address recipient, uint256 bips, bytes memory path) external;
-
-    /// @dev Trades against Curve (uint256 variants) using the contracts balance for funding
-    // Pre-req: Funded
-    // Post-req: Payout
-    function CURVE_UINT256_EXCHANGE(
-        address pool,
-        address sellToken,
-        uint256 fromTokenIndex,
-        uint256 toTokenIndex,
-        uint256 sellAmount,
-        uint256 minBuyAmount
-    ) external;
-
-    function TRANSFER_OUT_FIXED(address token, address recipient, uint256 amount) external;
+    function UNISWAPV2_SWAP(address recipient, uint256 bips, uint256 amountOutMin, bytes memory path) external;
 
-    function TRANSFER_OUT_POSITIVE_SLIPPAGE(address token, address recipient, uint256 expectedAmount) external;
+    function POSITIVE_SLIPPAGE(address token, address recipient, uint256 expectedAmount) external;
 
     // @dev Fill a 0x V4 OTC order using the 0x Exchange Proxy contract
     // Pre-req: Funded

src/Settler.sol

@@ -6,7 +6,6 @@ import {ISignatureTransfer} from "permit2/src/interfaces/ISignatureTransfer.sol"
 
 import {Permit2Payment} from "./core/Permit2Payment.sol";
 import {Basic} from "./core/Basic.sol";
-import {CurveV2} from "./core/CurveV2.sol";
 import {OtcOrderSettlement} from "./core/OtcOrderSettlement.sol";
 import {UniswapV3} from "./core/UniswapV3.sol";
 import {UniswapV2} from "./core/UniswapV2.sol";
@@ -72,7 +71,7 @@ library CalldataDecoder {
     }
 }
 
-contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, UniswapV2, CurveV2, ZeroEx, FreeMemory {
+contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, UniswapV2, ZeroEx, FreeMemory {
     using SafeTransferLib for ERC20;
     using SafeTransferLib for address payable;
     using UnsafeMath for uint256;
@@ -107,7 +106,7 @@ contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, Uniswa
         Basic()
         OtcOrderSettlement()
         UniswapV3(uniFactory, poolInitCodeHash)
-        CurveV2()
+        UniswapV2()
         ZeroEx(zeroEx)
     {
         assert(ACTIONS_AND_SLIPPAGE_TYPEHASH == keccak256(bytes(ACTIONS_AND_SLIPPAGE_TYPE)));
@@ -168,7 +167,7 @@ contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, Uniswa
                         bytes
                     )
                 );
-                fillOtcOrder(makerPermit, maker, makerSig, takerPermit, takerSig, slippage.recipient);
+                fillOtcOrder(slippage.recipient, makerPermit, maker, makerSig, takerPermit, takerSig);
                 return;
             } else if (action == ISettlerActions.UNISWAPV3_PERMIT2_SWAP_EXACT_IN.selector) {
                 (
@@ -234,9 +233,10 @@ contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, Uniswa
         internal
         DANGEROUS_freeMemory
     {
-        ISignatureTransfer.PermitTransferFrom memory permit = abi.decode(data, (ISignatureTransfer.PermitTransferFrom));
+        (address recipient, ISignatureTransfer.PermitTransferFrom memory permit) =
+            abi.decode(data, (address, ISignatureTransfer.PermitTransferFrom));
         (ISignatureTransfer.SignatureTransferDetails memory transferDetails,,) =
-            _permitToTransferDetails(permit, address(this));
+            _permitToTransferDetails(permit, recipient);
 
         // We simultaneously transfer-in the taker's tokens and authenticate the
         // metatransaction.
@@ -289,7 +289,7 @@ contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, Uniswa
                 ) = abi.decode(
                     data, (ISignatureTransfer.PermitTransferFrom, address, bytes, ISignatureTransfer.PermitTransferFrom)
                 );
-                fillOtcOrderMetaTxn(makerPermit, maker, makerSig, takerPermit, msgSender, sig, slippage.recipient);
+                fillOtcOrderMetaTxn(slippage.recipient, makerPermit, maker, makerSig, takerPermit, msgSender, sig);
                 return;
             } else if (action == ISettlerActions.METATXN_PERMIT2_TRANSFER_FROM.selector) {
                 // Checking this witness ensures that the entire sequence of actions is
@@ -320,54 +320,38 @@ contract Settler is Permit2Payment, Basic, OtcOrderSettlement, UniswapV3, Uniswa
         DANGEROUS_freeMemory
     {
         if (action == ISettlerActions.PERMIT2_TRANSFER_FROM.selector) {
-            (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory sig) =
-                abi.decode(data, (ISignatureTransfer.PermitTransferFrom, bytes));
+            (address recipient, ISignatureTransfer.PermitTransferFrom memory permit, bytes memory sig) =
+                abi.decode(data, (address, ISignatureTransfer.PermitTransferFrom, bytes));
             (ISignatureTransfer.SignatureTransferDetails memory transferDetails,,) =
-                _permitToTransferDetails(permit, address(this));
+                _permitToTransferDetails(permit, recipient);
             _permit2TransferFrom(permit, transferDetails, msgSender, sig);
         } else if (action == ISettlerActions.SETTLER_OTC_SELF_FUNDED.selector) {
             (
+                address recipient,
                 ISignatureTransfer.PermitTransferFrom memory permit,
                 address maker,
-                bytes memory sig,
+                bytes memory makerSig,
                 ERC20 takerToken,
                 uint256 maxTakerAmount
-            ) = abi.decode(data, (ISignatureTransfer.PermitTransferFrom, address, bytes, ERC20, uint256));
+            ) = abi.decode(data, (address, ISignatureTransfer.PermitTransferFrom, address, bytes, ERC20, uint256));
 
-            fillOtcOrderSelfFunded(permit, maker, sig, takerToken, maxTakerAmount, msgSender);
+            fillOtcOrderSelfFunded(recipient, permit, maker, makerSig, takerToken, maxTakerAmount, msgSender);
         } else if (action == ISettlerActions.UNISWAPV3_SWAP_EXACT_IN.selector) {
             (address recipient, uint256 bips, uint256 amountOutMin, bytes memory path) =
                 abi.decode(data, (address, uint256, uint256, bytes));
 
             sellTokenForTokenToUniswapV3(path, bips, amountOutMin, recipient);
         } else if (action == ISettlerActions.UNISWAPV2_SWAP.selector) {
-            (address recipient, uint256 bips, bytes memory path) = abi.decode(data, (address, uint256, bytes));
+            (address recipient, uint256 bips, uint256 amountOutMin, bytes memory path) =
+                abi.decode(data, (address, uint256, uint256, bytes));
 
-            sellToUniswapV2(path, bips, recipient);
-        } else if (action == ISettlerActions.CURVE_UINT256_EXCHANGE.selector) {
-            (
-                address pool,
-                ERC20 sellToken,
-                uint256 fromTokenIndex,
-                uint256 toTokenIndex,
-                uint256 sellAmount,
-                uint256 minBuyAmount
-            ) = abi.decode(data, (address, ERC20, uint256, uint256, uint256, uint256));
-
-            sellTokenForTokenToCurve(pool, sellToken, fromTokenIndex, toTokenIndex, sellAmount, minBuyAmount);
+            sellToUniswapV2(path, bips, amountOutMin, recipient);
         } else if (action == ISettlerActions.BASIC_SELL.selector) {
             (address pool, ERC20 sellToken, uint256 proportion, uint256 offset, bytes memory _data) =
                 abi.decode(data, (address, ERC20, uint256, uint256, bytes));
 
             basicSellToPool(pool, sellToken, proportion, offset, _data);
-        } else if (action == ISettlerActions.TRANSFER_OUT_FIXED.selector) {
-            (ERC20 token, address recipient, uint256 amount) = abi.decode(data, (ERC20, address, uint256));
-            if (token == ERC20(ETH_ADDRESS)) {
-                payable(recipient).safeTransferETH(amount);
-            } else {
-                token.safeTransfer(recipient, amount);
-            }
-        } else if (action == ISettlerActions.TRANSFER_OUT_POSITIVE_SLIPPAGE.selector) {
+        } else if (action == ISettlerActions.POSITIVE_SLIPPAGE.selector) {
             (ERC20 token, address recipient, uint256 expectedAmount) = abi.decode(data, (ERC20, address, uint256));
             if (token == ERC20(ETH_ADDRESS)) {
                 uint256 balance = address(this).balance;

src/core/Basic.sol

@@ -9,9 +9,24 @@ import {SafeTransferLib} from "../utils/SafeTransferLib.sol";
 import {FullMath} from "../utils/FullMath.sol";
 import {Panic} from "../utils/Panic.sol";
 
+library Revert {
+    function _revert(bytes memory reason) internal pure {
+        assembly ("memory-safe") {
+            revert(add(reason, 0x20), mload(reason))
+        }
+    }
+
+    function maybeRevert(bool success, bytes memory reason) internal pure {
+        if (!success) {
+            _revert(reason);
+        }
+    }
+}
+
 abstract contract Basic is Permit2PaymentAbstract {
     using SafeTransferLib for ERC20;
     using FullMath for uint256;
+    using Revert for bool;
 
     address internal constant ETH_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
 
@@ -23,29 +38,41 @@ abstract contract Basic is Permit2PaymentAbstract {
         if (isAllowanceHolder(pool)) {
             revert ConfusedDeputy();
         }
-        if ((offset += 32) > data.length) {
-            Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
-        }
 
         uint256 value;
-        uint256 amount;
         if (sellToken == ERC20(ETH_ADDRESS)) {
-            value = amount = address(this).balance.mulDiv(bips, 10_000);
+            value = address(this).balance.mulDiv(bips, 10_000);
+            if (data.length == 0) {
+                require(offset == 0);
+                (bool success, bytes memory returnData) = payable(pool).call{value: value}("");
+                success.maybeRevert(returnData);
+                return;
+            } else {
+                if ((offset += 32) > data.length) {
+                    Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
+                }
+                assembly ("memory-safe") {
+                    mstore(add(data, offset), value)
+                }
+            }
         } else {
-            amount = sellToken.balanceOf(address(this)).mulDiv(bips, 10_000);
-            if (pool != address(sellToken)) {
-                sellToken.safeApproveIfBelow(pool, amount);
+            if (address(sellToken) == address(0)) {
+                require(offset == 0);
+            } else {
+                uint256 amount = sellToken.balanceOf(address(this)).mulDiv(bips, 10_000);
+                if ((offset += 32) > data.length) {
+                    Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
+                }
+                assembly ("memory-safe") {
+                    mstore(add(data, offset), amount)
+                }
+                if (address(sellToken) != pool) {
+                    sellToken.safeApproveIfBelow(pool, amount);
+                }
             }
         }
-        assembly ("memory-safe") {
-            mstore(add(data, offset), amount)
-        }
-        // We omit the EXTCODESIZE check here deliberately. This can be used to send value to EOAs.
         (bool success, bytes memory returnData) = payable(pool).call{value: value}(data);
-        if (!success) {
-            assembly ("memory-safe") {
-                revert(add(0x20, returnData), mload(returnData))
-            }
-        }
+        success.maybeRevert(returnData);
+        require(returnData.length > 0 || pool.code.length > 0); // forbid sending data to EOAs
     }
 }