From 444877accbca3d8e6c4bc0f2b09148c6268f9058 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 12:40:02 +0400 Subject: [PATCH 01/23] wip on reentrant calculate orders --- src/concrete/OrderBook.sol | 87 ++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 50fefc1df..5664383d6 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -453,7 +453,7 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash remainingTakerInput -= takerInput; totalTakerOutput += takerOutput; - recordVaultIO(order, takerOutput, takerInput, orderIOCalculation); + recordVaultIO(takerOutput, takerInput, orderIOCalculation); emit TakeOrder(msg.sender, takeOrderConfig, takerInput, takerOutput); } } @@ -504,82 +504,82 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash /// @inheritdoc IOrderBookV3 function clear( - Order memory alice, - Order memory bob, + Order memory aliceOrder, + Order memory bobOrder, ClearConfig calldata clearConfig, SignedContextV1[] memory aliceSignedContext, SignedContextV1[] memory bobSignedContext ) external nonReentrant { { - if (alice.owner == bob.owner) { - revert SameOwner(alice.owner); + if (aliceOrder.owner == bobOrder.owner) { + revert SameOwner(aliceOrder.owner); } if ( - alice.validOutputs[clearConfig.aliceOutputIOIndex].token - != bob.validInputs[clearConfig.bobInputIOIndex].token + aliceOrder.validOutputs[clearConfig.aliceOutputIOIndex].token + != bobOrder.validInputs[clearConfig.bobInputIOIndex].token ) { revert TokenMismatch( - alice.validOutputs[clearConfig.aliceOutputIOIndex].token, - bob.validInputs[clearConfig.bobInputIOIndex].token + aliceOrder.validOutputs[clearConfig.aliceOutputIOIndex].token, + bobOrder.validInputs[clearConfig.bobInputIOIndex].token ); } if ( - alice.validOutputs[clearConfig.aliceOutputIOIndex].decimals - != bob.validInputs[clearConfig.bobInputIOIndex].decimals + aliceOrder.validOutputs[clearConfig.aliceOutputIOIndex].decimals + != bobOrder.validInputs[clearConfig.bobInputIOIndex].decimals ) { revert TokenDecimalsMismatch( - alice.validOutputs[clearConfig.aliceOutputIOIndex].decimals, - bob.validInputs[clearConfig.bobInputIOIndex].decimals + aliceOrder.validOutputs[clearConfig.aliceOutputIOIndex].decimals, + bobOrder.validInputs[clearConfig.bobInputIOIndex].decimals ); } if ( - bob.validOutputs[clearConfig.bobOutputIOIndex].token - != alice.validInputs[clearConfig.aliceInputIOIndex].token + bobOrder.validOutputs[clearConfig.bobOutputIOIndex].token + != aliceOrder.validInputs[clearConfig.aliceInputIOIndex].token ) { revert TokenMismatch( - alice.validInputs[clearConfig.aliceInputIOIndex].token, - bob.validOutputs[clearConfig.bobOutputIOIndex].token + aliceOrder.validInputs[clearConfig.aliceInputIOIndex].token, + bobOrder.validOutputs[clearConfig.bobOutputIOIndex].token ); } if ( - bob.validOutputs[clearConfig.bobOutputIOIndex].decimals - != alice.validInputs[clearConfig.aliceInputIOIndex].decimals + bobOrder.validOutputs[clearConfig.bobOutputIOIndex].decimals + != aliceOrder.validInputs[clearConfig.aliceInputIOIndex].decimals ) { revert TokenDecimalsMismatch( - alice.validInputs[clearConfig.aliceInputIOIndex].decimals, - bob.validOutputs[clearConfig.bobOutputIOIndex].decimals + aliceOrder.validInputs[clearConfig.aliceInputIOIndex].decimals, + bobOrder.validOutputs[clearConfig.bobOutputIOIndex].decimals ); } // If either order is dead the clear is a no-op other than emitting // `OrderNotFound`. Returning rather than erroring makes it easier to // bulk clear using `Multicall`. - if (sOrders[alice.hash()] == ORDER_DEAD) { - emit OrderNotFound(msg.sender, alice.owner, alice.hash()); + if (sOrders[aliceOrder.hash()] == ORDER_DEAD) { + emit OrderNotFound(msg.sender, aliceOrder.owner, aliceOrder.hash()); return; } - if (sOrders[bob.hash()] == ORDER_DEAD) { - emit OrderNotFound(msg.sender, bob.owner, bob.hash()); + if (sOrders[bobOrder.hash()] == ORDER_DEAD) { + emit OrderNotFound(msg.sender, bobOrder.owner, bobOrder.hash()); return; } // Emit the Clear event before `eval`. - emit Clear(msg.sender, alice, bob, clearConfig); + emit Clear(msg.sender, aliceOrder, bobOrder, clearConfig); } OrderIOCalculation memory aliceOrderIOCalculation = calculateOrderIO( - alice, clearConfig.aliceInputIOIndex, clearConfig.aliceOutputIOIndex, bob.owner, bobSignedContext + aliceOrder, clearConfig.aliceInputIOIndex, clearConfig.aliceOutputIOIndex, bobOrder.owner, bobSignedContext ); OrderIOCalculation memory bobOrderIOCalculation = calculateOrderIO( - bob, clearConfig.bobInputIOIndex, clearConfig.bobOutputIOIndex, alice.owner, aliceSignedContext + bobOrder, clearConfig.bobInputIOIndex, clearConfig.bobOutputIOIndex, aliceOrder.owner, aliceSignedContext ); ClearStateChange memory clearStateChange = calculateClearStateChange(aliceOrderIOCalculation, bobOrderIOCalculation); - recordVaultIO(alice, clearStateChange.aliceInput, clearStateChange.aliceOutput, aliceOrderIOCalculation); - recordVaultIO(bob, clearStateChange.bobInput, clearStateChange.bobOutput, bobOrderIOCalculation); + recordVaultIO(clearStateChange.aliceInput, clearStateChange.aliceOutput, aliceOrderIOCalculation); + recordVaultIO(clearStateChange.bobInput, clearStateChange.bobOutput, bobOrderIOCalculation); { // At least one of these will overflow due to negative bounties if @@ -587,16 +587,19 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash uint256 aliceBounty = clearStateChange.aliceOutput - clearStateChange.bobInput; uint256 bobBounty = clearStateChange.bobOutput - clearStateChange.aliceInput; if (aliceBounty > 0) { - sVaultBalances[msg.sender][alice.validOutputs[clearConfig.aliceOutputIOIndex].token][clearConfig + sVaultBalances[msg.sender][aliceOrder.validOutputs[clearConfig.aliceOutputIOIndex].token][clearConfig .aliceBountyVaultId] += aliceBounty; } if (bobBounty > 0) { - sVaultBalances[msg.sender][bob.validOutputs[clearConfig.bobOutputIOIndex].token][clearConfig + sVaultBalances[msg.sender][bobOrder.validOutputs[clearConfig.bobOutputIOIndex].token][clearConfig .bobBountyVaultId] += bobBounty; } } emit AfterClear(msg.sender, clearStateChange); + + handleIO(aliceOrderIOCalculation); + handleIO(bobOrderIOCalculation); } /// Main entrypoint into an order calculates the amount and IO ratio. Both @@ -705,7 +708,6 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash /// Given an order, final input and output amounts and the IO calculation /// verbatim from `_calculateOrderIO`, dispatch the handle IO entrypoint if /// it exists and update the order owner's vault balances. - /// @param order The order that is being cleared. /// @param input The exact token input amount to move into the owner's /// vault. /// @param output The exact token output amount to move out of the owner's @@ -713,7 +715,6 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash /// @param orderIOCalculation The verbatim order IO calculation returned by /// `_calculateOrderIO`. function recordVaultIO( - Order memory order, uint256 input, uint256 output, OrderIOCalculation memory orderIOCalculation @@ -723,13 +724,13 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash if (input > 0) { // IMPORTANT! THIS MATH MUST BE CHECKED TO AVOID OVERFLOW. - sVaultBalances[order.owner][address( + sVaultBalances[orderIOCalculation.order.owner][address( uint160(orderIOCalculation.context[CONTEXT_VAULT_INPUTS_COLUMN][CONTEXT_VAULT_IO_TOKEN]) )][orderIOCalculation.context[CONTEXT_VAULT_INPUTS_COLUMN][CONTEXT_VAULT_IO_VAULT_ID]] += input; } if (output > 0) { // IMPORTANT! THIS MATH MUST BE CHECKED TO AVOID UNDERFLOW. - sVaultBalances[order.owner][address( + sVaultBalances[orderIOCalculation.order.owner][address( uint160(orderIOCalculation.context[CONTEXT_VAULT_OUTPUTS_COLUMN][CONTEXT_VAULT_IO_TOKEN]) )][orderIOCalculation.context[CONTEXT_VAULT_OUTPUTS_COLUMN][CONTEXT_VAULT_IO_VAULT_ID]] -= output; } @@ -737,7 +738,9 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // Emit the context only once in its fully populated form rather than two // nearly identical emissions of a partial and full context. emit Context(msg.sender, orderIOCalculation.context); + } + function handleIO(OrderIOCalculation memory orderIOCalculation) internal { // Apply state changes to the interpreter store after the vault balances // are updated, but before we call handle IO. We want handle IO to see // a consistent view on sets from calculate IO. @@ -747,12 +750,12 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // failing calls and resubmit a new transaction. // https://github.com/crytic/slither/issues/880 //slither-disable-next-line calls-loop - order.evaluable.store.set(orderIOCalculation.namespace, orderIOCalculation.kvs); + orderIOCalculation.order.evaluable.store.set(orderIOCalculation.namespace, orderIOCalculation.kvs); } // Only dispatch handle IO entrypoint if it is defined, otherwise it is // a waste of gas to hit the interpreter a second time. - if (order.handleIO) { + if (orderIOCalculation.order.handleIO) { // The handle IO eval is run under the same namespace as the // calculate order entrypoint. // Slither false positive. External calls within loops are fine if @@ -760,10 +763,10 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // failing calls and resubmit a new transaction. // https://github.com/crytic/slither/issues/880 //slither-disable-next-line calls-loop - (uint256[] memory handleIOStack, uint256[] memory handleIOKVs) = order.evaluable.interpreter.eval( - order.evaluable.store, + (uint256[] memory handleIOStack, uint256[] memory handleIOKVs) = orderIOCalculation.order.evaluable.interpreter.eval( + orderIOCalculation.order.evaluable.store, orderIOCalculation.namespace, - _handleIODispatch(order.evaluable.expression), + _handleIODispatch(orderIOCalculation.order.evaluable.expression), orderIOCalculation.context ); // There's nothing to be done with the stack. @@ -776,7 +779,7 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // drop failing calls and resubmit a new transaction. // https://github.com/crytic/slither/issues/880 //slither-disable-next-line calls-loop - order.evaluable.store.set(orderIOCalculation.namespace, handleIOKVs); + orderIOCalculation.order.evaluable.store.set(orderIOCalculation.namespace, handleIOKVs); } } } From 7cffaf500c4ff653db8314b26cdbebac7b8f320a Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 13:05:47 +0400 Subject: [PATCH 02/23] slither reentrancy --- src/concrete/OrderBook.sol | 251 +++++++++++++++++++++---------------- 1 file changed, 142 insertions(+), 109 deletions(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 5664383d6..85a940647 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -344,125 +344,152 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash revert NoOrders(); } - uint256 i = 0; TakeOrderConfig memory takeOrderConfig; Order memory order; - uint256 remainingTakerInput = config.maximumInput; - if (remainingTakerInput == 0) { - revert ZeroMaximumInput(); - } - while (i < config.orders.length && remainingTakerInput > 0) { - takeOrderConfig = config.orders[i]; - order = takeOrderConfig.order; - // Every order needs the same input token. - if ( - order.validInputs[takeOrderConfig.inputIOIndex].token - != config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token - ) { - revert TokenMismatch( - order.validInputs[takeOrderConfig.inputIOIndex].token, - config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token - ); - } - // Every order needs the same output token. - if ( - order.validOutputs[takeOrderConfig.outputIOIndex].token - != config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token - ) { - revert TokenMismatch( - order.validOutputs[takeOrderConfig.outputIOIndex].token, - config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token - ); - } - // Every order needs the same input token decimals. - if ( - order.validInputs[takeOrderConfig.inputIOIndex].decimals - != config.orders[0].order.validInputs[config.orders[0].inputIOIndex].decimals - ) { - revert TokenDecimalsMismatch( - order.validInputs[takeOrderConfig.inputIOIndex].decimals, - config.orders[0].order.validInputs[config.orders[0].inputIOIndex].decimals - ); - } - // Every order needs the same output token decimals. - if ( - order.validOutputs[takeOrderConfig.outputIOIndex].decimals - != config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].decimals - ) { - revert TokenDecimalsMismatch( - order.validOutputs[takeOrderConfig.outputIOIndex].decimals, - config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].decimals - ); + // Allocate a region of pointers but don't initialize it. Not even with + // the length. We'll do that later when we know how many orders we're + // actually going to handle. + OrderIOCalculation[] memory orderIOCalculationsToHandle; + { + uint256 length = config.orders.length; + assembly ("memory-safe") { + let ptr := mload(0x40) + orderIOCalculationsToHandle := ptr + mstore(0x40, add(ptr, mul(add(length, 1), 0x20))) } + } - bytes32 orderHash = order.hash(); - if (sOrders[orderHash] == ORDER_DEAD) { - emit OrderNotFound(msg.sender, order.owner, orderHash); - } else { - OrderIOCalculation memory orderIOCalculation = calculateOrderIO( - order, - takeOrderConfig.inputIOIndex, - takeOrderConfig.outputIOIndex, - msg.sender, - takeOrderConfig.signedContext - ); + { + uint256 remainingTakerInput = config.maximumInput; + if (remainingTakerInput == 0) { + revert ZeroMaximumInput(); + } + uint256 i = 0; + while (i < config.orders.length && remainingTakerInput > 0) { + takeOrderConfig = config.orders[i]; + order = takeOrderConfig.order; + // Every order needs the same input token. + if ( + order.validInputs[takeOrderConfig.inputIOIndex].token + != config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token + ) { + revert TokenMismatch( + order.validInputs[takeOrderConfig.inputIOIndex].token, + config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token + ); + } + // Every order needs the same output token. + if ( + order.validOutputs[takeOrderConfig.outputIOIndex].token + != config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token + ) { + revert TokenMismatch( + order.validOutputs[takeOrderConfig.outputIOIndex].token, + config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token + ); + } + // Every order needs the same input token decimals. + if ( + order.validInputs[takeOrderConfig.inputIOIndex].decimals + != config.orders[0].order.validInputs[config.orders[0].inputIOIndex].decimals + ) { + revert TokenDecimalsMismatch( + order.validInputs[takeOrderConfig.inputIOIndex].decimals, + config.orders[0].order.validInputs[config.orders[0].inputIOIndex].decimals + ); + } + // Every order needs the same output token decimals. + if ( + order.validOutputs[takeOrderConfig.outputIOIndex].decimals + != config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].decimals + ) { + revert TokenDecimalsMismatch( + order.validOutputs[takeOrderConfig.outputIOIndex].decimals, + config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].decimals + ); + } - // Skip orders that are too expensive rather than revert as we have - // no way of knowing if a specific order becomes too expensive - // between submitting to mempool and execution, but other orders may - // be valid so we want to take advantage of those if possible. - if (orderIOCalculation.IORatio > config.maximumIORatio) { - emit OrderExceedsMaxRatio(msg.sender, order.owner, orderHash); - } else if (Output18Amount.unwrap(orderIOCalculation.outputMax) == 0) { - emit OrderZeroAmount(msg.sender, order.owner, orderHash); + bytes32 orderHash = order.hash(); + if (sOrders[orderHash] == ORDER_DEAD) { + emit OrderNotFound(msg.sender, order.owner, orderHash); } else { - uint8 takerInputDecimals = order.validOutputs[takeOrderConfig.outputIOIndex].decimals; - // Taker is just "market buying" the order output max. - Input18Amount takerInput18 = Input18Amount.wrap(Output18Amount.unwrap(orderIOCalculation.outputMax)); - // Cap the taker input at the remaining input before - // calculating the taker output. Keep everything in 18 - // decimals at this point, which requires rescaling the - // remaining taker input to match. - { - // Round down and saturate when converting remaining taker input to 18 decimals. - Input18Amount remainingTakerInput18 = - Input18Amount.wrap(remainingTakerInput.scale18(takerInputDecimals, FLAG_SATURATE)); - if (Input18Amount.unwrap(takerInput18) > Input18Amount.unwrap(remainingTakerInput18)) { - takerInput18 = remainingTakerInput18; + OrderIOCalculation memory orderIOCalculation = calculateOrderIO( + order, + takeOrderConfig.inputIOIndex, + takeOrderConfig.outputIOIndex, + msg.sender, + takeOrderConfig.signedContext + ); + + // Skip orders that are too expensive rather than revert as we have + // no way of knowing if a specific order becomes too expensive + // between submitting to mempool and execution, but other orders may + // be valid so we want to take advantage of those if possible. + if (orderIOCalculation.IORatio > config.maximumIORatio) { + emit OrderExceedsMaxRatio(msg.sender, order.owner, orderHash); + } else if (Output18Amount.unwrap(orderIOCalculation.outputMax) == 0) { + emit OrderZeroAmount(msg.sender, order.owner, orderHash); + } else { + uint8 takerInputDecimals = order.validOutputs[takeOrderConfig.outputIOIndex].decimals; + // Taker is just "market buying" the order output max. + Input18Amount takerInput18 = + Input18Amount.wrap(Output18Amount.unwrap(orderIOCalculation.outputMax)); + // Cap the taker input at the remaining input before + // calculating the taker output. Keep everything in 18 + // decimals at this point, which requires rescaling the + // remaining taker input to match. + { + // Round down and saturate when converting remaining taker input to 18 decimals. + Input18Amount remainingTakerInput18 = + Input18Amount.wrap(remainingTakerInput.scale18(takerInputDecimals, FLAG_SATURATE)); + if (Input18Amount.unwrap(takerInput18) > Input18Amount.unwrap(remainingTakerInput18)) { + takerInput18 = remainingTakerInput18; + } } - } - uint256 takerOutput; - { - // Always round IO calculations up so the taker pays more. - Output18Amount takerOutput18 = Output18Amount.wrap( - // Use the capped taker input to calculate the taker - // output. - Input18Amount.unwrap(takerInput18).fixedPointMul( - orderIOCalculation.IORatio, Math.Rounding.Up - ) - ); - takerOutput = Output18Amount.unwrap(takerOutput18).scaleN( - order.validInputs[takeOrderConfig.inputIOIndex].decimals, FLAG_ROUND_UP - ); - } + uint256 takerOutput; + { + // Always round IO calculations up so the taker pays more. + Output18Amount takerOutput18 = Output18Amount.wrap( + // Use the capped taker input to calculate the taker + // output. + Input18Amount.unwrap(takerInput18).fixedPointMul( + orderIOCalculation.IORatio, Math.Rounding.Up + ) + ); + takerOutput = Output18Amount.unwrap(takerOutput18).scaleN( + order.validInputs[takeOrderConfig.inputIOIndex].decimals, FLAG_ROUND_UP + ); + } - uint256 takerInput = Input18Amount.unwrap(takerInput18).scaleN(takerInputDecimals, FLAG_SATURATE); + uint256 takerInput = + Input18Amount.unwrap(takerInput18).scaleN(takerInputDecimals, FLAG_SATURATE); - remainingTakerInput -= takerInput; - totalTakerOutput += takerOutput; + remainingTakerInput -= takerInput; + totalTakerOutput += takerOutput; - recordVaultIO(takerOutput, takerInput, orderIOCalculation); - emit TakeOrder(msg.sender, takeOrderConfig, takerInput, takerOutput); + recordVaultIO(takerOutput, takerInput, orderIOCalculation); + emit TakeOrder(msg.sender, takeOrderConfig, takerInput, takerOutput); + + // Add the pointer to the order IO calculation to the array + // of order IO calculations to handle. + assembly ("memory-safe") { + // Inc the length by 1. + let newLength := add(mload(orderIOCalculationsToHandle), 1) + mstore(orderIOCalculationsToHandle, newLength) + // Store the pointer to the order IO calculation. + mstore(add(orderIOCalculationsToHandle, mul(newLength, 0x20)), orderIOCalculation) + } + } } - } - unchecked { - i++; + unchecked { + i++; + } } + totalTakerInput = config.maximumInput - remainingTakerInput; } - totalTakerInput = config.maximumInput - remainingTakerInput; if (totalTakerInput < config.minimumInput) { revert MinimumInput(config.minimumInput, totalTakerInput); @@ -500,6 +527,12 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash msg.sender, address(this), totalTakerOutput ); } + + unchecked { + for (uint256 i = 0; i < orderIOCalculationsToHandle.length; i++) { + handleIO(orderIOCalculationsToHandle[i]); + } + } } /// @inheritdoc IOrderBookV3 @@ -714,11 +747,7 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash /// vault. /// @param orderIOCalculation The verbatim order IO calculation returned by /// `_calculateOrderIO`. - function recordVaultIO( - uint256 input, - uint256 output, - OrderIOCalculation memory orderIOCalculation - ) internal { + function recordVaultIO(uint256 input, uint256 output, OrderIOCalculation memory orderIOCalculation) internal { orderIOCalculation.context[CONTEXT_VAULT_INPUTS_COLUMN][CONTEXT_VAULT_IO_BALANCE_DIFF] = input; orderIOCalculation.context[CONTEXT_VAULT_OUTPUTS_COLUMN][CONTEXT_VAULT_IO_BALANCE_DIFF] = output; @@ -763,7 +792,11 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // failing calls and resubmit a new transaction. // https://github.com/crytic/slither/issues/880 //slither-disable-next-line calls-loop - (uint256[] memory handleIOStack, uint256[] memory handleIOKVs) = orderIOCalculation.order.evaluable.interpreter.eval( + (uint256[] memory handleIOStack, uint256[] memory handleIOKVs) = orderIOCalculation + .order + .evaluable + .interpreter + .eval( orderIOCalculation.order.evaluable.store, orderIOCalculation.namespace, _handleIODispatch(orderIOCalculation.order.evaluable.expression), From 91cdd60cb3a5d15cfa4164b4587f98924f72f8d6 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 14:28:51 +0400 Subject: [PATCH 03/23] wip on slither --- src/abstract/OrderBookV3ArbCommon.sol | 2 +- src/abstract/OrderBookV3ArbOrderTaker.sol | 2 +- src/abstract/OrderBookV3FlashBorrower.sol | 23 ++- src/abstract/OrderBookV3FlashLender.sol | 157 +----------------- src/concrete/OrderBook.sol | 42 ++--- .../ierc3156/IERC3156FlashLender.sol | 2 +- 6 files changed, 37 insertions(+), 191 deletions(-) diff --git a/src/abstract/OrderBookV3ArbCommon.sol b/src/abstract/OrderBookV3ArbCommon.sol index 2fdde39b6..267eeae36 100644 --- a/src/abstract/OrderBookV3ArbCommon.sol +++ b/src/abstract/OrderBookV3ArbCommon.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: CAL -pragma solidity ^0.8.18; +pragma solidity ^0.8.19; /// Thrown when the minimum output for the sender is not met after the arb. /// @param minimum The minimum output expected by the sender. diff --git a/src/abstract/OrderBookV3ArbOrderTaker.sol b/src/abstract/OrderBookV3ArbOrderTaker.sol index 4b148267c..42c25845a 100644 --- a/src/abstract/OrderBookV3ArbOrderTaker.sol +++ b/src/abstract/OrderBookV3ArbOrderTaker.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: CAL -pragma solidity =0.8.19; +pragma solidity ^0.8.19; import {ERC165, IERC165} from "lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol"; import {ReentrancyGuard} from "lib/openzeppelin-contracts/contracts/security/ReentrancyGuard.sol"; diff --git a/src/abstract/OrderBookV3FlashBorrower.sol b/src/abstract/OrderBookV3FlashBorrower.sol index d12ae846c..7401d4bf2 100644 --- a/src/abstract/OrderBookV3FlashBorrower.sol +++ b/src/abstract/OrderBookV3FlashBorrower.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: CAL -pragma solidity =0.8.19; +pragma solidity ^0.8.19; import {ERC165, IERC165} from "lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol"; import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; @@ -12,13 +12,20 @@ import { DeployerDiscoverableMetaV2ConstructionConfig, LibMeta } from "lib/rain.interpreter/src/abstract/DeployerDiscoverableMetaV2.sol"; -import "lib/rain.interpreter/src/lib/caller/LibEncodedDispatch.sol"; -import "lib/rain.interpreter/src/lib/caller/LibContext.sol"; -import "lib/rain.interpreter/src/lib/bytecode/LibBytecode.sol"; - -import "../interface/unstable/IOrderBookV3.sol"; -import "lib/rain.factory/src/interface/ICloneableV2.sol"; -import "./OrderBookV3ArbCommon.sol"; +import {LibEncodedDispatch, EncodedDispatch} from "lib/rain.interpreter/src/lib/caller/LibEncodedDispatch.sol"; +import {LibContext} from "lib/rain.interpreter/src/lib/caller/LibContext.sol"; +import {LibBytecode} from "lib/rain.interpreter/src/lib/bytecode/LibBytecode.sol"; +import {ON_FLASH_LOAN_CALLBACK_SUCCESS} from "../interface/ierc3156/IERC3156FlashBorrower.sol"; +import {EvaluableConfigV2} from "rain.interpreter/src/lib/caller/LibEvaluable.sol"; +import {IOrderBookV3, TakeOrdersConfigV2, NoOrders} from "../interface/unstable/IOrderBookV3.sol"; +import {ICloneableV2, ICLONEABLE_V2_SUCCESS} from "lib/rain.factory/src/interface/ICloneableV2.sol"; +import { + IInterpreterV1, SourceIndex, DEFAULT_STATE_NAMESPACE +} from "lib/rain.interpreter/src/interface/IInterpreterV1.sol"; +import {IERC3156FlashBorrower} from "../interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IInterpreterStoreV1} from "lib/rain.interpreter/src/interface/IInterpreterStoreV1.sol"; +import {BadLender, MinimumOutput, NonZeroBeforeArbStack, Initializing} from "./OrderBookV3ArbCommon.sol"; +import {SignedContextV1} from "rain.interpreter/src/interface/IInterpreterCallerV2.sol"; /// Thrown when the initiator is not the order book. /// @param badInitiator The untrusted initiator of the flash loan. diff --git a/src/abstract/OrderBookV3FlashLender.sol b/src/abstract/OrderBookV3FlashLender.sol index 54f173492..066015889 100644 --- a/src/abstract/OrderBookV3FlashLender.sol +++ b/src/abstract/OrderBookV3FlashLender.sol @@ -1,33 +1,17 @@ // SPDX-License-Identifier: CAL -pragma solidity ^0.8.18; +pragma solidity ^0.8.19; -import {Math} from "lib/openzeppelin-contracts/contracts/utils/math/Math.sol"; import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; -import "../interface/ierc3156/IERC3156FlashBorrower.sol"; -import "../interface/ierc3156/IERC3156FlashLender.sol"; - -/// Thrown when `flashLoan` token is zero address. -error ZeroToken(); - -/// Thrown when `flashLoan` receiver is zero address. -error ZeroReceiver(); - -/// Thrown when `flashLoan` amount is zero. -error ZeroAmount(); +import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "../interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC3156FlashLender} from "../interface/ierc3156/IERC3156FlashLender.sol"; /// Thrown when the `onFlashLoan` callback returns anything other than /// ON_FLASH_LOAN_CALLBACK_SUCCESS. /// @param result The value that was returned by `onFlashLoan`. error FlashLenderCallbackFailed(bytes32 result); -/// Thrown when more than one debt is attempted simultaneously. -/// @param receiver The receiver of the active debt. -/// @param token The token of the active debt. -/// @param amount The amount of the active debt. -error ActiveDebt(address receiver, address token, uint256 amount); - /// @dev Flash fee is always 0 for orderbook as there's no entity to take /// revenue for `Orderbook` and its more important anyway that flashloans happen /// to connect external liquidity to live orders via arbitrage. @@ -36,151 +20,24 @@ uint256 constant FLASH_FEE = 0; /// @title OrderBookV3FlashLender /// @notice Implements `IERC3156FlashLender` for `OrderBook`. Based on the /// reference implementation by Alberto Cuesta Cañada found at -/// https://eips.ethereum.org/EIPS/eip-3156 -/// Several features found in the reference implementation are simplified or -/// hardcoded for `OrderBookV3`. +/// https://eips.ethereum.org/EIPS/eip-3156#flash-loan-reference-implementation abstract contract OrderBookV3FlashLender is IERC3156FlashLender { - using Math for uint256; using SafeERC20 for IERC20; - IERC3156FlashBorrower private _sReceiver = IERC3156FlashBorrower(address(0)); - address private _sToken = address(0); - uint256 private _sAmountUnpaid = 0; - - /// If any of the debt related storage values are set then we consider a - /// debt active. Notably the debt is still active even if the amount unpaid - /// is zero, until the loan originating `flashLoan` call fully completes. - function _isActiveDebt() internal view returns (bool) { - return (address(_sReceiver) != address(0)) || (_sToken != address(0)) || (_sAmountUnpaid != 0); - } - - function _checkActiveDebt() internal view { - if (_isActiveDebt()) { - revert ActiveDebt(address(_sReceiver), _sToken, _sAmountUnpaid); - } - } - - /// Whenever `Orderbook` sends tokens to any address it MUST first attempt - /// to decrease any outstanding flash loans for that address. Consider the - /// case that Alice deposits 100 TKN and she is the only depositor of TKN - /// then flash borrows 100 TKN. If she attempts to withdraw 100 TKN during - /// her `onFlashLoan` callback then `Orderbook`: - /// - /// - has 0 TKN balance to process the withdrawal - /// - MUST process the withdrawal as Alice has the right to withdraw her - /// balance at any time - /// - Has the 100 TKN debt active under Alice - /// - /// In this case `Orderbook` can simply forgive Alice's 100 TKN debt instead - /// of actually transferring any tokens. The withdrawal can decrease her - /// vault balance by 100 TKN decoupled from needing to know whether a - /// tranfer or forgiveness happened. - /// - /// The same logic applies to withdrawals as sending tokens during - /// `takeOrders` as the reason for sending tokens is irrelevant, all that - /// matters is that `Orderbook` prioritises debt repayments over external - /// transfers. - /// - /// If there is an active debt that only partially eclipses the withdrawal - /// then the debt will be fully repaid and the remainder transferred as a - /// real token transfer. - /// - /// Note that Alice can still contrive a situation that causes `Orderbook` - /// to attempt to send tokens that it does not have. If Alice can write a - /// smart contract to trigger withdrawals she can flash loan 100% of the - /// TKN supply in `Orderbook` and trigger her contract to attempt a - /// withdrawal. For any normal ERC20 token this will fail and revert as the - /// `Orderbook` cannot send tokens it does not have under any circumstances, - /// but the scenario is worth being aware of for more exotic token - /// behaviours that may not be supported. - /// - /// @param token The token being sent or for the debt being paid. - /// @param receiver The receiver of the token or holder of the debt. - /// @param sendAmount The amount to send or repay. - /// @return The final amount sent after any debt repayment. - function _decreaseFlashDebtThenSendToken(address token, address receiver, uint256 sendAmount) - internal - returns (uint256) - { - // If this token transfer matches the active debt then prioritise - // reducing debt over sending tokens. - if (token == _sToken && receiver == address(_sReceiver)) { - uint256 debtReduction = sendAmount.min(_sAmountUnpaid); - sendAmount -= debtReduction; - - // Even if this completely zeros the amount the debt is considered - // active until the `flashLoan` also clears the token and recipient. - _sAmountUnpaid -= debtReduction; - } - - if (sendAmount > 0) { - IERC20(token).safeTransfer(receiver, sendAmount); - } - return sendAmount; - } - /// @inheritdoc IERC3156FlashLender function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 amount, bytes calldata data) external override returns (bool) { - // Set the active debt before transferring tokens to prevent reeentrancy. - // The active debt is set beyond the scope of `flashLoan` to facilitate - // early repayment via. `_decreaseFlashDebtThenSendToken`. - { - if (address(receiver) == address(0)) { - revert ZeroReceiver(); - } - if (token == address(0)) { - revert ZeroToken(); - } - if (amount == 0) { - revert ZeroAmount(); - } - - // This prevents reentrancy, loans can be taken sequentially within - // a transaction but not simultanously. If any of these values are - // nonzero in storage, they cannot be set to a new value. - _checkActiveDebt(); - _sToken = token; - _sReceiver = receiver; - // As long as FLASH_FEE is 0 this `+` will probably get optimised - // away by the compiler. - _sAmountUnpaid = amount + FLASH_FEE; - IERC20(token).safeTransfer(address(receiver), amount); - } + IERC20(token).safeTransfer(address(receiver), amount); bytes32 result = receiver.onFlashLoan(msg.sender, token, amount, FLASH_FEE, data); if (result != ON_FLASH_LOAN_CALLBACK_SUCCESS) { revert FlashLenderCallbackFailed(result); } - // Pull tokens before releasing the active debt to prevent a new loan - // from being taken reentrantly during the repayment of the current loan. - { - // Sync local `amount_` with global `_amount` in case an early - // repayment was made during the loan term via. - // `_decreaseFlashDebtThenSendToken`. - amount = _sAmountUnpaid; - if (amount > 0) { - // There is no way to fix this slither warning and be compatible - // with ERC3156. - // https://github.com/crytic/slither/issues/1658 - //slither-disable-next-line arbitrary-send-erc20 - IERC20(token).safeTransferFrom(address(receiver), address(this), amount); - _sAmountUnpaid = 0; - } - - // Both of these are required to fully clear the active debt and - // allow new debts. - _sReceiver = IERC3156FlashBorrower(address(0)); - _sToken = address(0); - } - - // Guard against some bad code path that allowed an active debt to remain - // at this point. Should be impossible. - _checkActiveDebt(); + IERC20(token).safeTransferFrom(address(receiver), address(this), amount + FLASH_FEE); return true; } @@ -195,6 +52,6 @@ abstract contract OrderBookV3FlashLender is IERC3156FlashLender { /// then loans are disabled so the max becomes `0` until after repayment. /// @inheritdoc IERC3156FlashLender function maxFlashLoan(address token) external view override returns (uint256) { - return _isActiveDebt() ? 0 : IERC20(token).balanceOf(address(this)); + return IERC20(token).balanceOf(address(this)); } } diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 85a940647..a8f170dcf 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -207,32 +207,17 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash DeployerDiscoverableMetaV2(CALLER_META_HASH, config) {} - /// Guard against read-only reentrancy. - /// https://chainsecurity.com/heartbreaks-curve-lp-oracles/ - modifier nonReentrantView() { - if (_reentrancyGuardEntered()) { - revert ReentrancyGuardReentrantCall(); - } - _; - } - /// @inheritdoc IOrderBookV3 // This has a reentrancy guard on it but Slither doesn't know that because // it is a read-only reentrancy due to potential cross function reentrancy. // https://github.com/crytic/slither/issues/735#issuecomment-1620704314 //slither-disable-next-line reentrancy-no-eth - function vaultBalance(address owner, address token, uint256 vaultId) - external - view - override - nonReentrantView - returns (uint256) - { + function vaultBalance(address owner, address token, uint256 vaultId) external view override returns (uint256) { return sVaultBalances[owner][token][vaultId]; } /// @inheritdoc IOrderBookV3 - function orderExists(bytes32 orderHash) external view override nonReentrantView returns (bool) { + function orderExists(bytes32 orderHash) external view override returns (bool) { return sOrders[orderHash] == ORDER_LIVE; } @@ -264,7 +249,7 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // withdrawals to exceed vault balances. sVaultBalances[msg.sender][token][vaultId] = currentVaultBalance - withdrawAmount; emit Withdraw(msg.sender, token, vaultId, targetAmount, withdrawAmount); - _decreaseFlashDebtThenSendToken(token, msg.sender, withdrawAmount); + IERC20(token).safeTransfer(msg.sender, withdrawAmount); } } @@ -495,34 +480,31 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash revert MinimumInput(config.minimumInput, totalTakerInput); } - // Prioritise paying down any active flash loans before sending any - // tokens to `msg.sender`. We send the tokens to `msg.sender` first - // adopting a similar pattern to Uniswap flash swaps. We call the caller - // before attempting to pull tokens from them in order to facilitate - // better integrations with external liquidity sources. This could be - // done by the caller using flash loans but this callback: + // We send the tokens to `msg.sender` first adopting a similar pattern to + // Uniswap flash swaps. We call the caller before attempting to pull + // tokens from them in order to facilitate better integrations with + // external liquidity sources. This could be done by the caller using + // flash loans but this callback: // - may be simpler for the caller to implement // - allows the caller to call `takeOrders` _before_ placing external // trades, which is important if the order logic itself is dependent on // external data (e.g. prices) that could be modified by the caller's // trades. - uint256 takerInputAmountSent = _decreaseFlashDebtThenSendToken( - config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token, msg.sender, totalTakerInput + + IERC20(config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token).safeTransfer( + msg.sender, totalTakerInput ); if (config.data.length > 0) { IOrderBookV3OrderTaker(msg.sender).onTakeOrders( config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token, config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token, - takerInputAmountSent, + totalTakerInput, totalTakerOutput, config.data ); } if (totalTakerOutput > 0) { - // We already updated vault balances before we took tokens from - // `msg.sender` which is usually NOT the correct order of operations for - // depositing to a vault. We rely on reentrancy guards to make this safe. IERC20(config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token).safeTransferFrom( msg.sender, address(this), totalTakerOutput ); diff --git a/src/interface/ierc3156/IERC3156FlashLender.sol b/src/interface/ierc3156/IERC3156FlashLender.sol index ff1dd954e..cbbe25345 100644 --- a/src/interface/ierc3156/IERC3156FlashLender.sol +++ b/src/interface/ierc3156/IERC3156FlashLender.sol @@ -2,7 +2,7 @@ // Alberto Cuesta Cañada, Fiona Kobayashi, fubuloubu, Austin Williams, "EIP-3156: Flash Loans," Ethereum Improvement Proposals, no. 3156, November 2020. [Online serial]. Available: https://eips.ethereum.org/EIPS/eip-3156. pragma solidity ^0.8.18; -import "./IERC3156FlashBorrower.sol"; +import {IERC3156FlashBorrower} from "./IERC3156FlashBorrower.sol"; interface IERC3156FlashLender { /** From 021f38f3d60fa28089b8eff2e30705f0dddd0aec Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 14:49:27 +0400 Subject: [PATCH 04/23] slither false positive --- src/abstract/OrderBookV3FlashLender.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/abstract/OrderBookV3FlashLender.sol b/src/abstract/OrderBookV3FlashLender.sol index 066015889..5e47875cb 100644 --- a/src/abstract/OrderBookV3FlashLender.sol +++ b/src/abstract/OrderBookV3FlashLender.sol @@ -37,6 +37,18 @@ abstract contract OrderBookV3FlashLender is IERC3156FlashLender { revert FlashLenderCallbackFailed(result); } + // This behaviour is copied almost verbatim from the ERC3156 spec. + // Slither is complaining because this kind of logic can normally be used + // to grief the token holder. Consider if they were to approve order book + // for the sake of depositing and then someone could cause them to send + // tokens to order book without their consent. However, in this case the + // flash loan spec provides two reasons that this is not a problem: + // - We just sent this exact amount to the receiver as part of the loan, + // so transferring them back with a 0 fee is net neutral. + // - The receiver is a contract that has explicitly opted in to this + // behaviour by implementing `IERC3156FlashBorrower`. + // https://github.com/crytic/slither/issues/1658 + //slither-disable-next-line arbitrary-send-erc20 IERC20(token).safeTransferFrom(address(receiver), address(this), amount + FLASH_FEE); return true; From 2c5c1b4dabdb2318a5a5263fcb5713908a55141c Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 14:58:51 +0400 Subject: [PATCH 05/23] slither false positive --- src/concrete/OrderBook.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index a8f170dcf..ef341c99a 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -320,6 +320,9 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash } /// @inheritdoc IOrderBookV3 + // Most of the cyclomatic complexity here is due to the error handling within + // the loop. The actual logic is fairly linear. + //slither-disable-next-line cyclomatic-complexity function takeOrders(TakeOrdersConfigV2 calldata config) external nonReentrant From e27650f2768d84c2f4ad5f7021bdf2261d12f77d Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Wed, 8 Nov 2023 15:04:16 +0400 Subject: [PATCH 06/23] add docs --- src/abstract/OrderBookV3FlashLender.sol | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/abstract/OrderBookV3FlashLender.sol b/src/abstract/OrderBookV3FlashLender.sol index 5e47875cb..460188641 100644 --- a/src/abstract/OrderBookV3FlashLender.sol +++ b/src/abstract/OrderBookV3FlashLender.sol @@ -39,14 +39,16 @@ abstract contract OrderBookV3FlashLender is IERC3156FlashLender { // This behaviour is copied almost verbatim from the ERC3156 spec. // Slither is complaining because this kind of logic can normally be used - // to grief the token holder. Consider if they were to approve order book - // for the sake of depositing and then someone could cause them to send + // to grief the token holder. Consider if alice were to approve order book + // for the sake of depositing and then bob could cause alice to send // tokens to order book without their consent. However, in this case the // flash loan spec provides two reasons that this is not a problem: - // - We just sent this exact amount to the receiver as part of the loan, - // so transferring them back with a 0 fee is net neutral. + // - We just sent this exact amount to the receiver as the loan, so + // transferring them back with a 0 fee is net neutral. // - The receiver is a contract that has explicitly opted in to this - // behaviour by implementing `IERC3156FlashBorrower`. + // behaviour by implementing `IERC3156FlashBorrower`. The success check + // for `onFlashLoan` guarantees the receiver has opted into this + // behaviour independently of any approvals, etc. // https://github.com/crytic/slither/issues/1658 //slither-disable-next-line arbitrary-send-erc20 IERC20(token).safeTransferFrom(address(receiver), address(this), amount + FLASH_FEE); From a90e944f9021b01726edd01ffef24d45e55bdad6 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 11:23:10 +0400 Subject: [PATCH 07/23] fix zero transfer error --- src/concrete/OrderBook.sol | 46 +++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index ef341c99a..636b5b234 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -7,21 +7,38 @@ import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; import {ReentrancyGuard} from "openzeppelin-contracts/contracts/security/ReentrancyGuard.sol"; -import "rain.math.fixedpoint/lib/LibFixedPointDecimalArithmeticOpenZeppelin.sol"; -import "rain.math.fixedpoint/lib/LibFixedPointDecimalScale.sol"; -import "rain.interpreter/src/lib/caller/LibEncodedDispatch.sol"; -import "rain.interpreter/src/lib/caller/LibContext.sol"; +import {FLAG_SATURATE, FLAG_ROUND_UP} from "rain.math.fixedpoint/lib/FixedPointDecimalConstants.sol"; +import {LibFixedPointDecimalArithmeticOpenZeppelin} from + "rain.math.fixedpoint/lib/LibFixedPointDecimalArithmeticOpenZeppelin.sol"; +import {LibFixedPointDecimalScale} from "rain.math.fixedpoint/lib/LibFixedPointDecimalScale.sol"; +import {LibEncodedDispatch, EncodedDispatch} from "rain.interpreter/src/lib/caller/LibEncodedDispatch.sol"; +import {LibContext} from "rain.interpreter/src/lib/caller/LibContext.sol"; import { DeployerDiscoverableMetaV2, DeployerDiscoverableMetaV2ConstructionConfig, LibMeta } from "rain.interpreter/src/abstract/DeployerDiscoverableMetaV2.sol"; -import "rain.interpreter/src/lib/bytecode/LibBytecode.sol"; - -import "../interface/unstable/IOrderBookV3.sol"; -import "../interface/unstable/IOrderBookV3OrderTaker.sol"; -import "../lib/LibOrder.sol"; -import "../abstract/OrderBookV3FlashLender.sol"; +import {LibBytecode} from "rain.interpreter/src/lib/bytecode/LibBytecode.sol"; +import {SourceIndex, StateNamespace, IInterpreterV1} from "rain.interpreter/src/interface/IInterpreterV1.sol"; +import { + IOrderBookV3, + NoOrders, + Order, + OrderConfigV2, + TakeOrderConfig, + TakeOrdersConfigV2, + ClearConfig, + ClearStateChange, + ZeroMaximumInput +} from "../interface/unstable/IOrderBookV3.sol"; +import {IOrderBookV3OrderTaker} from "../interface/unstable/IOrderBookV3OrderTaker.sol"; +import {LibOrder} from "../lib/LibOrder.sol"; +import {OrderBookV3FlashLender} from "../abstract/OrderBookV3FlashLender.sol"; +import {LibUint256Array} from "rain.solmem/lib/LibUint256Array.sol"; +import {SignedContextV1} from "rain.interpreter/src/interface/IInterpreterCallerV2.sol"; +import {Evaluable} from "rain.interpreter/src/lib/caller/LibEvaluable.sol"; +import {IInterpreterStoreV1} from "rain.interpreter/src/interface/IInterpreterStoreV1.sol"; +import {IExpressionDeployerV2} from "rain.interpreter/src/interface/unstable/IExpressionDeployerV2.sol"; /// This will exist in a future version of Open Zeppelin if their main branch is /// to be believed. @@ -494,9 +511,12 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // external data (e.g. prices) that could be modified by the caller's // trades. - IERC20(config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token).safeTransfer( - msg.sender, totalTakerInput - ); + if (totalTakerInput > 0) { + IERC20(config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token).safeTransfer( + msg.sender, totalTakerInput + ); + } + if (config.data.length > 0) { IOrderBookV3OrderTaker(msg.sender).onTakeOrders( config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token, From eb3f7f0cb71b5783818e8917f9bd4273c2e76b4a Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 15:22:08 +0400 Subject: [PATCH 08/23] rebuild meta --- meta/OrderBook.rain.meta | Bin 5795 -> 5752 bytes test/concrete/OrderBook.vaultBalance.t.sol | 17 ------------- test/concrete/OrderBook.withdraw.t.sol | 28 --------------------- 3 files changed, 45 deletions(-) diff --git a/meta/OrderBook.rain.meta b/meta/OrderBook.rain.meta index eccb4ad6fd855c5b909dff0b54a14fee007c0eec..2166c8a72dd593637f9ce85ed155ddbfaed51243 100644 GIT binary patch delta 1594 zcmV-A2F3ZKE%+>d{|br5bnbX`r2tt57wue2Z`(K!{wsygz9dD9ptt6Q8fYpY$!38K ze4r&d;YK12l5%V;`rmg%OD6TEB*%4PUfNhBXE;Ohn+HdF{BK0)B9>zG4gU05v|tp| z^e3SbbBeY=uw#SR@O26KK=4lvnG0c)@E>>&?O2D2^t{hzh=A zi|N)3)l(bjIOtV$ldiPEg<2h}?OhtnUTNh}bCOcc(#!X>nQ7rL#kB4? zX;!z0P<3QZ-byK&of7L8`+;Z>;)%C6hLa7^eX;X@V!2m-cP!>U9?GwMW8T4k!#8E} z(`NxLZA%MK5K7VsC}!0N4I7Pk^SQthR>Pa0vzP`QHI&dj)4Ty5eSFVeFulY+iBKr= zQ#h-Wz@RZ>vt|lfgj~NHA|fRH=YdPCMd*iJ+4LKCiR#gB!X%~p#2%F zR>Q=5oSFn6Rw;UQN(a#ExU^7mHn(a}R2nfOrK;RW<0$hS2I;NULak(b6`@ws`#lJM zygWy|goWELxHj*X($XnMYNPpn(JgpVLxwB<1A=Q{KdFze?RL#Yh!Nl;Bup*<#Ged& z^2;ryOiQ--b0DoK9q{ryW9N*WGj<=-*eN94!Pat+;2^<4!iRtaaE`iftOgft;?*TA zZ9Tn>jDzT89^X(RJD7?RDM`r6^+WA{7Oy3$0Wek5lvP zBxsLUayBkN za{-zQ&_3V=#VwB55+B?hg~LCGe-8iNJ^l^DyJi8%;EWGzPxo|ONkN`$foPkOut{53 z)Rv2KadTUNc~TFJgER^9aAHnHm`y{h+dX>OSQpDW^&TNQX}T)zMvB#4TN@RCnLsXU zv;&39$^x`E&O>F@Z&fQ;|6+)LGi}baS*pRdTye;|MBOXR3#LX$M5U-SHtp`<2F-J& zm-!|Oi$3V(D8r_hv+&z4G>|UHV(9``)oSOW1fi>af?}j#{a)2Prs{pI7L;+3j*E1h zIrxx~&gQneF5GcA;Bdg< zuI)tWy|b1CzjhWyDcnSVz>4+Aqh16sLVOg4Eh|za%++UWL^bW5E>F)pdbX*0BSKAb zx0}swZCr5bf>Rfq9`AMh)BsDfUg$lf&`YWw{`sz`@hrtem9R|CS zC|447z$H<#y#&gn$qFBAG*3*w=(J@DqQ{lF+z##-A4(>n5k~Vc0hUq|@fwP=-ZqN1 sx25~Zi%s)=&QXY{|5kaSUeWu({%(Nz7by3)$G~x7rvrh(85c?q+YybcN delta 1637 zcmV-r2AcW!ETb)d{|br5bnbX`r2tt5LhW2jZ`(K!{woWgb4iL8L2r3(4Ky|2WV65q zKG4!Q;YK12k#cMl{qG&omPx%S$#I>8mo}EjA?KlQ9vtc6=YX<#EXCj}{^{{>&Ka=H zPs$`jj0AV9ISjrIi0?-r#NcSaNSIvkVIJ@mM0iDJhx|H!D$Jg)Bp7o;kVMa0-~2T> z%Fhxqiaq()41dm&5$#H6xISRVQLJ6C~9O_g2Oe@eJwYNkBZn@dEZ}GqWcAfn6 zxDN%(w!jskgf&FjE^EN3(PE5`IgiCUj2?PZLlN?h;RR2_6keAhKIH zt>VCcpfTmsdJ39_%(xpWLL$AVo+{QXbc4Ti`?Xf0Ms(Y7!Q;Ym4xR-68+z3ezkAe7 zfp{_|LB)K%>3>+dR05~j;k6Ise-QDrTnw^zx6~tkS`Z9rhJNkSl<z*i5o&ZnFuGsiU1WNO=mAGr86bLMK&o%` zkx4BrOeoMhVLmgwTF#S6n6#v+&3*PU&)qFwu4TSf<VESMG{3{- z1~P^Js@QBB7jG|H?W>gFE#_xnQWeRCZbqvmUykYJ>FE`w8}TN0ObHp6vgv1gy|sIP zl1|M8y_I9Oe+J9NFdRKhOahpK<%3lv1040Sq)>>Vw_;FKYB6I&RYk|fqa0;0NSjsz zwL(0g2sMTSXTCqmt_TZM+88kR-TUOTJsuj?rD8>1zERH3M9JSY0--vzm(l z2;w6ayDkvK9}Rr+ig&WMWGH`zq!p!q16IFtcFx&3XZJpxokG$bn3jVC2MG=m-UTFJ zaMa$gYC^Q(sH%u+8|kfi>_sPy`1%sr!B!MYNs(Bt?ki2eZZ}O-+6A+PGv3U_su1?i zN3RXEHqv;Qn9ojv_OL7_0BV%UU4Go*@z)N`Y(!;)Q5}d`oS$z>zi+^t3|jGj^nWbp zcao81=SF&voj-m(d|RxI3tyw0qMX^)(gW}X9tezyq})lYQDS(&f1R9LI#D%>s`MDn z)ofgX<`OiQpuNKbiZO(I0S9+R;qcGlpToa5kAMB}uATwfyW+!}%RMbMDd?ju5N%5m zHf4*-4z=vkxt{H&mjjYEZ}2yNMUeYrb1TBU)yKQtk>|B{v3$p%LyAu8j*8on;#J4i zMpt6WkgFQ)K;W{p0qvFRP-*vDnFgz0>T#~kxi(8N*tRS7c@M08_j}IO2uYX}rOu`~ z99*M$ru8y=%fh4gda=#$DdsM`rUMP48?yLD!Dap9nJhu*ZXZ9NDOkUM6E};Qde^Q2 zWn8A?G9Bj*-esoax^Wx?I0*RA5a7CRYKXkS?i;y_ZkrEWW>WgPZKySUVELdOHc4la zwXsZI(>poYEtt0{BlRxw_}gZqJ<6k-C~EnbB_9I02B=S1Eom0HpJzyz3hEv1d`oUk_agPqq5x=K#-5t8M#9V?wJYeC_q zH7iQsCIXh|Ba3<=!U*-&U#+OqNS<({zQH5PY421$9Z%-@y4Q^gwaMKYo1Jc4Uh48v zmzRFrL->gSmS&gGTPU0tMBV>eWMSzSNV-rM3-^BlkAvt j5)kISR$Zu5_I@z+!G^XnEy)+%3f8MkIQb2;BnDIvL^m?n diff --git a/test/concrete/OrderBook.vaultBalance.t.sol b/test/concrete/OrderBook.vaultBalance.t.sol index c26439ce2..64ee6f35e 100644 --- a/test/concrete/OrderBook.vaultBalance.t.sol +++ b/test/concrete/OrderBook.vaultBalance.t.sol @@ -13,21 +13,4 @@ contract OrderBookDepositTest is OrderBookExternalMockTest { function testVaultBalanceNoDeposits(address token, uint256 vaultId) external { assertEq(iOrderbook.vaultBalance(address(this), token, vaultId), 0); } - - /// Test that depositing can't reentrantly read the vault balance. - function testVaultBalanceReentrant( - address alice, - uint256 vaultIdAlice, - uint256 amount, - address bob, - address tokenBob, - uint256 vaultIdBob - ) external { - vm.assume(amount > 0); - vm.prank(alice); - Reenteroor reenteroor = new Reenteroor(); - reenteroor.reenterWith(abi.encodeWithSelector(IOrderBookV3.vaultBalance.selector, bob, tokenBob, vaultIdBob)); - vm.expectRevert(ReentrancyGuardReentrantCall.selector); - iOrderbook.deposit(address(reenteroor), vaultIdAlice, amount); - } } diff --git a/test/concrete/OrderBook.withdraw.t.sol b/test/concrete/OrderBook.withdraw.t.sol index f8349c1bc..892d23b75 100644 --- a/test/concrete/OrderBook.withdraw.t.sol +++ b/test/concrete/OrderBook.withdraw.t.sol @@ -190,32 +190,4 @@ contract OrderBookWithdrawTest is OrderBookExternalMockTest { } } } - - /// Test that withdrawing can't reentrantly read the vault balance. - function testWithdrawReentrant( - address alice, - uint256 vaultIdAlice, - uint256 amount, - address bob, - address tokenBob, - uint256 vaultIdBob - ) external { - vm.assume(amount > 0); - Reenteroor reenteroor = new Reenteroor(); - reenteroor.reenterWith(abi.encodeWithSelector(IOrderBookV3.vaultBalance.selector, bob, tokenBob, vaultIdBob)); - - // Withdraw short circuits if there's no vault balance so first we need - // to deposit. - vm.prank(alice); - vm.mockCall( - address(reenteroor), - abi.encodeWithSelector(IERC20.transferFrom.selector, alice, address(iOrderbook), amount), - abi.encode(true) - ); - iOrderbook.deposit(address(reenteroor), vaultIdAlice, amount); - - vm.prank(alice); - vm.expectRevert(ReentrancyGuardReentrantCall.selector); - iOrderbook.withdraw(address(reenteroor), vaultIdAlice, amount); - } } From 9d2050a4c342234842e2e29e2b77cce9902807a7 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 15:30:51 +0400 Subject: [PATCH 09/23] fix hash --- src/concrete/OrderBook.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 636b5b234..4a54d0356 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -140,7 +140,7 @@ uint256 constant CONTEXT_VAULT_IO_BALANCE_DIFF = 4; uint256 constant CONTEXT_VAULT_IO_ROWS = 5; /// @dev Hash of the caller contract metadata for construction. -bytes32 constant CALLER_META_HASH = bytes32(0x1317ffd909f4ca1cd6402c7dd02501ba5965a5b8787a9627cde7b5f3f8f6f840); +bytes32 constant CALLER_META_HASH = bytes32(0xd6912c2a900b10b78c9c43592b8690b48c804fb78bbdcb3ceee1f56db830d137); /// All information resulting from an order calculation that allows for vault IO /// to be calculated and applied, then the handle IO entrypoint to be dispatched. From f28bac59da01c7c98bc015d542ccfc4d986e65bb Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 17:32:42 +0400 Subject: [PATCH 10/23] mock tests for flash loans --- test/concrete/OrderBook.clear.mock.t.sol | 8 ++- ...rderBookV3FlashLender.griefRecipient.t.sol | 57 +++++++++++++++++++ .../OrderBookV3FlashLender.mockSuccess.t.sol | 30 ++++++++++ 3 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol create mode 100644 test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol diff --git a/test/concrete/OrderBook.clear.mock.t.sol b/test/concrete/OrderBook.clear.mock.t.sol index 29d17534a..37613f19b 100644 --- a/test/concrete/OrderBook.clear.mock.t.sol +++ b/test/concrete/OrderBook.clear.mock.t.sol @@ -1,10 +1,14 @@ // SPDX-License-Identifier: CAL pragma solidity =0.8.19; -import "lib/forge-std/src/Test.sol"; +import {Test} from "lib/forge-std/src/Test.sol"; -import "test/util/abstract/OrderBookExternalMockTest.sol"; +import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; +import {OrderConfigV2, Order, IO, ClearConfig} from "src/interface/unstable/IOrderBookV3.sol"; import {LibTestAddOrder} from "test/util/lib/LibTestAddOrder.sol"; +import {SignedContextV1} from "rain.interpreter/src/interface/IInterpreterCallerV2.sol"; +import {IInterpreterV1} from "rain.interpreter/src/interface/IInterpreterV1.sol"; /// @title OrderBookClearTest /// Tests clearing an order. diff --git a/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol b/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol new file mode 100644 index 000000000..0265c51af --- /dev/null +++ b/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; +import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; +import {FlashLenderCallbackFailed} from "src/abstract/OrderBookV3FlashLender.sol"; + +/// @title OrderBookV3FlashLenderGriefRecipientTest +/// Try to grief the recipient of the flash loan. +contract OrderBookClearTest is OrderBookExternalMockTest { + /// Tests that no matter who the receiver is, and no matter what happens with + /// the tokens, the flash loan will revert if the receiver is not + /// `OrderBookV3FlashBorrower`. + function testFlashLoanToNonReceiver( + uint256 amount, + bytes memory data, + bytes32 notFlashLoanSuccess, + bytes memory notFlashLoanSuccessBytes + ) public { + vm.assume(notFlashLoanSuccess != ON_FLASH_LOAN_CALLBACK_SUCCESS); + vm.assume(keccak256(notFlashLoanSuccessBytes) != keccak256(abi.encode(ON_FLASH_LOAN_CALLBACK_SUCCESS))); + + // Return true for all transfers. + vm.mockCall(address(iToken0), abi.encodeWithSelector(IERC20.transfer.selector), abi.encode(true)); + vm.mockCall(address(iToken0), abi.encodeWithSelector(IERC20.transferFrom.selector), abi.encode(true)); + + // A call to an EOA will revert with no data. + address receiver = address(0xDEADBEEF); + vm.expectRevert(); + iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); + + // A call to a contract that does not implement `IERC3156FlashBorrower` + // will revert with `FlashLenderCallbackFailed`. + vm.etch(receiver, hex"FE"); + vm.mockCall( + receiver, + abi.encodeWithSelector(IERC3156FlashBorrower.onFlashLoan.selector), + abi.encode(notFlashLoanSuccess) + ); + vm.expectRevert(abi.encodeWithSelector(FlashLenderCallbackFailed.selector, notFlashLoanSuccess)); + iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); + + // A call to a contract that does not implement `IERC3156FlashBorrower` + // will revert with no data if the return value is not `bytes32`. + vm.mockCall( + receiver, abi.encodeWithSelector(IERC3156FlashBorrower.onFlashLoan.selector), notFlashLoanSuccessBytes + ); + vm.expectRevert(); + iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); + } + + /// Tests that if the receiver is some contract that returns + /// `ON_FLASH_LOAN_CALLBACK_SUCCESS`, and the token movements do not error, + /// then the flash loan will succeed. + +} diff --git a/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol new file mode 100644 index 000000000..04fc5deef --- /dev/null +++ b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; +import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; + +/// @title OrderBookV3FlashLenderMockSuccessTest +/// Show that if the receiver is `OrderBookV3FlashBorrower` and the token +/// movements do not error, then the flash loan will succeed. +contract OrderBookV3FlashLenderMockSuccessTest is OrderBookExternalMockTest { + /// Tests that if the receiver is `OrderBookV3FlashBorrower` and the token + /// movements do not error, then the flash loan will succeed. + function testFlashLoanToReceiver(uint256 amount, bytes memory data) public { + // Return true for all transfers. + vm.mockCall(address(iToken0), abi.encodeWithSelector(IERC20.transfer.selector), abi.encode(true)); + vm.mockCall(address(iToken0), abi.encodeWithSelector(IERC20.transferFrom.selector), abi.encode(true)); + + // A call to a contract that implements `IERC3156FlashBorrower` will + // succeed if the return value is `ON_FLASH_LOAN_CALLBACK_SUCCESS`. + address receiver = address(0xDEADBEEF); + vm.etch(receiver, hex"FE"); + vm.mockCall( + receiver, + abi.encodeWithSelector(IERC3156FlashBorrower.onFlashLoan.selector), + abi.encode(ON_FLASH_LOAN_CALLBACK_SUCCESS) + ); + iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); + } +} \ No newline at end of file From 5d187a0df23fb742e3ed409450729cce6fc812d9 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 18:00:34 +0400 Subject: [PATCH 11/23] handle io revert test --- .../OrderBook.takeOrder.handleIO.revert.t.sol | 51 +++++++++++++++++++ .../OrderBook.takeOrder.precision.t.sol | 2 +- ...rderBookV3FlashLender.griefRecipient.t.sol | 5 -- 3 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol new file mode 100644 index 000000000..0c06ca3f6 --- /dev/null +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {Vm} from "forge-std/Vm.sol"; +import {OrderBookExternalRealTest, OrderConfigV2, IO, IParserV1, EvaluableConfigV2, Order, TakeOrderConfig, SignedContextV1, TakeOrdersConfigV2} from "test/util/abstract/OrderBookExternalRealTest.sol"; +import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol"; + +/// @title OrderBookTakeOrderHandleIORevertTest +/// @notice A test harness for testing the OrderBook takeOrder function will run +/// handle IO and revert if it fails. +contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { + function testTakeOrderHandleIORevert() public { + uint256 vaultId = 0; + address inputToken = address(0x100); + address outputToken = address(0x101); + OrderConfigV2 memory config; + { + IO[] memory validInputs = new IO[](1); + validInputs[0] = IO(inputToken, 18, vaultId); + IO[] memory validOutputs = new IO[](1); + validOutputs[0] = IO(outputToken, 18, vaultId); + (bytes memory bytecode, uint256[] memory constants) = IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:ensure<1>(0);"); + EvaluableConfigV2 memory evaluableConfig = EvaluableConfigV2(iDeployer, bytecode, constants); + config = OrderConfigV2(validInputs, validOutputs, evaluableConfig, ""); + // Etch with invalid. + vm.etch(outputToken, hex"fe"); + vm.etch(inputToken, hex"fe"); + // Mock every call to output as a success, so the orderbook thinks it + // is transferring tokens. + vm.mockCall(outputToken, "", abi.encode(true)); + vm.mockCall(inputToken, "", abi.encode(true)); + } + iOrderbook.deposit(outputToken, vaultId, type(uint256).max); + assertEq(iOrderbook.vaultBalance(address(this), outputToken, vaultId), type(uint256).max); + + vm.recordLogs(); + iOrderbook.addOrder(config); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 3); + (,, Order memory order,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + TakeOrderConfig[] memory orders = new TakeOrderConfig[](1); + orders[0] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); + TakeOrdersConfigV2 memory takeOrdersConfig = + TakeOrdersConfigV2(0, type(uint256).max, type(uint256).max, orders, ""); + + vm.expectRevert(abi.encodeWithSelector(EnsureFailed.selector, 1, 0)); + (uint256 totalTakerInput, uint256 totalTakerOutput) = iOrderbook.takeOrders(takeOrdersConfig); + (totalTakerInput, totalTakerOutput); + } +} diff --git a/test/concrete/OrderBook.takeOrder.precision.t.sol b/test/concrete/OrderBook.takeOrder.precision.t.sol index 3ab3606eb..4dcaddcc0 100644 --- a/test/concrete/OrderBook.takeOrder.precision.t.sol +++ b/test/concrete/OrderBook.takeOrder.precision.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: CAL pragma solidity =0.8.19; -import "test/util/abstract/OrderBookExternalRealTest.sol"; +import {OrderBookExternalRealTest, OrderConfigV2, IO, EvaluableConfigV2, IParserV1, Vm, Order, TakeOrderConfig, SignedContextV1, TakeOrdersConfigV2} from "test/util/abstract/OrderBookExternalRealTest.sol"; /// @title OrderBookTakeOrderPrecisionTest /// @notice A test harness for testing the OrderBook takeOrder function. diff --git a/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol b/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol index 0265c51af..32a637b7f 100644 --- a/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol +++ b/test/concrete/OrderBookV3FlashLender.griefRecipient.t.sol @@ -49,9 +49,4 @@ contract OrderBookClearTest is OrderBookExternalMockTest { vm.expectRevert(); iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); } - - /// Tests that if the receiver is some contract that returns - /// `ON_FLASH_LOAN_CALLBACK_SUCCESS`, and the token movements do not error, - /// then the flash loan will succeed. - } From e8ff4b8f6dc9dfa37188b535594536afe3ac4e76 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Thu, 9 Nov 2023 18:10:40 +0400 Subject: [PATCH 12/23] fmt --- .../OrderBook.takeOrder.handleIO.revert.t.sol | 15 +++++++++++++-- test/concrete/OrderBook.takeOrder.precision.t.sol | 13 ++++++++++++- .../OrderBookV3FlashLender.mockSuccess.t.sol | 2 +- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol index 0c06ca3f6..001acba27 100644 --- a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -2,7 +2,17 @@ pragma solidity =0.8.19; import {Vm} from "forge-std/Vm.sol"; -import {OrderBookExternalRealTest, OrderConfigV2, IO, IParserV1, EvaluableConfigV2, Order, TakeOrderConfig, SignedContextV1, TakeOrdersConfigV2} from "test/util/abstract/OrderBookExternalRealTest.sol"; +import { + OrderBookExternalRealTest, + OrderConfigV2, + IO, + IParserV1, + EvaluableConfigV2, + Order, + TakeOrderConfig, + SignedContextV1, + TakeOrdersConfigV2 +} from "test/util/abstract/OrderBookExternalRealTest.sol"; import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol"; /// @title OrderBookTakeOrderHandleIORevertTest @@ -19,7 +29,8 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { validInputs[0] = IO(inputToken, 18, vaultId); IO[] memory validOutputs = new IO[](1); validOutputs[0] = IO(outputToken, 18, vaultId); - (bytes memory bytecode, uint256[] memory constants) = IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:ensure<1>(0);"); + (bytes memory bytecode, uint256[] memory constants) = + IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:ensure<1>(0);"); EvaluableConfigV2 memory evaluableConfig = EvaluableConfigV2(iDeployer, bytecode, constants); config = OrderConfigV2(validInputs, validOutputs, evaluableConfig, ""); // Etch with invalid. diff --git a/test/concrete/OrderBook.takeOrder.precision.t.sol b/test/concrete/OrderBook.takeOrder.precision.t.sol index 4dcaddcc0..9ef9ee4a2 100644 --- a/test/concrete/OrderBook.takeOrder.precision.t.sol +++ b/test/concrete/OrderBook.takeOrder.precision.t.sol @@ -1,7 +1,18 @@ // SPDX-License-Identifier: CAL pragma solidity =0.8.19; -import {OrderBookExternalRealTest, OrderConfigV2, IO, EvaluableConfigV2, IParserV1, Vm, Order, TakeOrderConfig, SignedContextV1, TakeOrdersConfigV2} from "test/util/abstract/OrderBookExternalRealTest.sol"; +import { + OrderBookExternalRealTest, + OrderConfigV2, + IO, + EvaluableConfigV2, + IParserV1, + Vm, + Order, + TakeOrderConfig, + SignedContextV1, + TakeOrdersConfigV2 +} from "test/util/abstract/OrderBookExternalRealTest.sol"; /// @title OrderBookTakeOrderPrecisionTest /// @notice A test harness for testing the OrderBook takeOrder function. diff --git a/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol index 04fc5deef..6b57df74c 100644 --- a/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol +++ b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol @@ -27,4 +27,4 @@ contract OrderBookV3FlashLenderMockSuccessTest is OrderBookExternalMockTest { ); iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); } -} \ No newline at end of file +} From 50c049ba248692db0def2052e85fbaf4aa7c352f Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 12:07:39 +0400 Subject: [PATCH 13/23] test transfers in flashloan --- .../concrete/OrderBookV3FlashLender.fee.t.sol | 13 ++ .../OrderBookV3FlashLender.maxFlashLoan.t.sol | 18 +++ .../OrderBookV3FlashLender.transfers.t.sol | 111 ++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 test/concrete/OrderBookV3FlashLender.fee.t.sol create mode 100644 test/concrete/OrderBookV3FlashLender.maxFlashLoan.t.sol create mode 100644 test/concrete/OrderBookV3FlashLender.transfers.t.sol diff --git a/test/concrete/OrderBookV3FlashLender.fee.t.sol b/test/concrete/OrderBookV3FlashLender.fee.t.sol new file mode 100644 index 000000000..be7658b29 --- /dev/null +++ b/test/concrete/OrderBookV3FlashLender.fee.t.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; + +/// @title OrderBookV3FlashLenderFeeTest +/// Tests the fee charged by `OrderBookV3FlashLender`. +contract OrderBookV3FlashLenderFeeTest is OrderBookExternalMockTest { + /// Tests that the fee charged by `OrderBookV3FlashLender` is 0. + function testFlashFee(address token, uint256 amount) public { + assertEq(iOrderbook.flashFee(token, amount), 0); + } +} diff --git a/test/concrete/OrderBookV3FlashLender.maxFlashLoan.t.sol b/test/concrete/OrderBookV3FlashLender.maxFlashLoan.t.sol new file mode 100644 index 000000000..d4426074b --- /dev/null +++ b/test/concrete/OrderBookV3FlashLender.maxFlashLoan.t.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; +import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; + +/// @title OrderBookV3FlashLenderMaxFlashLoanTest +/// Tests the maximum flash loan amount for `OrderBookV3FlashLender`. +contract OrderBookV3FlashLenderMaxFlashLoanTest is OrderBookExternalMockTest { + /// Tests that the maximum flash loan amount for `OrderBookV3FlashLender` is + /// the balance of the token in the order book. + function testFlashMaxLoan(uint256 amount) public { + vm.mockCall( + address(iToken0), abi.encodeWithSelector(IERC20.balanceOf.selector, address(iOrderbook)), abi.encode(amount) + ); + assertEq(iOrderbook.maxFlashLoan(address(iToken0)), amount); + } +} diff --git a/test/concrete/OrderBookV3FlashLender.transfers.t.sol b/test/concrete/OrderBookV3FlashLender.transfers.t.sol new file mode 100644 index 000000000..794661052 --- /dev/null +++ b/test/concrete/OrderBookV3FlashLender.transfers.t.sol @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {stdError} from "forge-std/Test.sol"; +import {OrderBookExternalMockTest} from "test/util/abstract/OrderBookExternalMockTest.sol"; +import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol"; +import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; +import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC3156FlashLender} from "src/interface/ierc3156/IERC3156FlashLender.sol"; +import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol"; +import {FlashLenderCallbackFailed} from "src/abstract/OrderBookV3FlashLender.sol"; + +contract TKN is ERC20 { + constructor(address recipient, uint256 supply) ERC20("TKN", "TKN") { + _mint(recipient, supply); + } +} + +interface IPull { + function pull(address token, uint256 amount) external; +} + +/// Alice has some daisy contract pull tokens from her. +/// If the tokens are returned to her then she can complete the flash loan else +/// the loan must be reverted. +contract Alice is IERC3156FlashBorrower { + IPull immutable iPull; + bool immutable iSuccess; + + constructor(IPull pull, bool success) { + iPull = pull; + iSuccess = success; + } + + function onFlashLoan(address, address token, uint256 amount, uint256, bytes calldata) + public + override + returns (bytes32) + { + // Approve the puller to pull the tokens. + IERC20(token).approve(address(iPull), amount); + iPull.pull(token, amount); + // Approve the lender to pull the tokens back and repay the loan. + IERC20(token).approve(msg.sender, amount); + // Magic number for success. + return iSuccess ? ON_FLASH_LOAN_CALLBACK_SUCCESS : bytes32(0); + } +} + +/// Bob pulls the tokens from Alice then returns them so she can repay the loan. +contract Bob is IPull { + using SafeERC20 for IERC20; + + function pull(address token, uint256 amount) public override { + IERC20(token).safeTransferFrom(msg.sender, address(this), amount); + IERC20(token).safeTransfer(msg.sender, amount); + } +} + +/// Carol pulls tokens from Alice then returns only some of them so the loan +/// fails. +contract Carol is IPull { + using SafeERC20 for IERC20; + + uint256 immutable iAmountWithheld; + + constructor(uint256 amountWithheld) { + iAmountWithheld = amountWithheld; + } + + function pull(address token, uint256 amount) public override { + IERC20(token).safeTransferFrom(msg.sender, address(this), amount); + IERC20(token).safeTransfer(msg.sender, amount - iAmountWithheld); + } +} + +/// @title OrderBookV3FlashLenderTransferTest +/// Tests the `OrderBookV3FlashLender` transfer functions. +contract OrderBookV3FlashLenderTransferTest is OrderBookExternalMockTest { + /// Alice can send tokens to Bob, who will return them and then the loan will + /// be repaid. + function testFlashLoanTransferSuccess(uint256 amount, bool success) public { + TKN tkn = new TKN(address(iOrderbook), amount); + + Bob bob = new Bob(); + Alice alice = new Alice(IPull(address(bob)), success); + + if (!success) { + vm.expectRevert(abi.encodeWithSelector(FlashLenderCallbackFailed.selector, bytes32(0))); + } + iOrderbook.flashLoan(IERC3156FlashBorrower(address(alice)), address(tkn), amount, ""); + } + + /// Alice can send tokens to Carol, who will return all but one of them and + /// then the loan will fail. + function testFlashLoanTransferFail(uint256 amount, uint256 amountWithheld, bool success) public { + amount = bound(amount, 1, type(uint256).max); + amountWithheld = bound(amountWithheld, 1, amount); + TKN tkn = new TKN(address(iOrderbook), amount); + + Carol carol = new Carol(amountWithheld); + Alice alice = new Alice(IPull(address(carol)), success); + + if (!success) { + vm.expectRevert(abi.encodeWithSelector(FlashLenderCallbackFailed.selector, bytes32(0))); + } else { + vm.expectRevert("ERC20: transfer amount exceeds balance"); + } + iOrderbook.flashLoan(IERC3156FlashBorrower(address(alice)), address(tkn), amount, ""); + } +} From 70ba0f040e796ad020d729fdae75daf0e4270061 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 12:56:04 +0400 Subject: [PATCH 14/23] stricter tests --- test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol | 2 +- test/concrete/OrderBookV3FlashLender.reentrant.t.sol | 0 test/concrete/OrderBookV3FlashLender.transfers.t.sol | 5 ++++- 3 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 test/concrete/OrderBookV3FlashLender.reentrant.t.sol diff --git a/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol index 6b57df74c..f11a7fde1 100644 --- a/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol +++ b/test/concrete/OrderBookV3FlashLender.mockSuccess.t.sol @@ -25,6 +25,6 @@ contract OrderBookV3FlashLenderMockSuccessTest is OrderBookExternalMockTest { abi.encodeWithSelector(IERC3156FlashBorrower.onFlashLoan.selector), abi.encode(ON_FLASH_LOAN_CALLBACK_SUCCESS) ); - iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data); + assertTrue(iOrderbook.flashLoan(IERC3156FlashBorrower(receiver), address(iToken0), amount, data)); } } diff --git a/test/concrete/OrderBookV3FlashLender.reentrant.t.sol b/test/concrete/OrderBookV3FlashLender.reentrant.t.sol new file mode 100644 index 000000000..e69de29bb diff --git a/test/concrete/OrderBookV3FlashLender.transfers.t.sol b/test/concrete/OrderBookV3FlashLender.transfers.t.sol index 794661052..6424f621d 100644 --- a/test/concrete/OrderBookV3FlashLender.transfers.t.sol +++ b/test/concrete/OrderBookV3FlashLender.transfers.t.sol @@ -88,7 +88,10 @@ contract OrderBookV3FlashLenderTransferTest is OrderBookExternalMockTest { if (!success) { vm.expectRevert(abi.encodeWithSelector(FlashLenderCallbackFailed.selector, bytes32(0))); } - iOrderbook.flashLoan(IERC3156FlashBorrower(address(alice)), address(tkn), amount, ""); + bool result = iOrderbook.flashLoan(IERC3156FlashBorrower(address(alice)), address(tkn), amount, ""); + if (success) { + assertTrue(result); + } } /// Alice can send tokens to Carol, who will return all but one of them and From e461457a6dd75cd7481ecff2d92f71fbe6564ba2 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 16:01:07 +0400 Subject: [PATCH 15/23] wip on reentrancy tests for flash loan --- .gas-snapshot | 126 ++++++++++-------- .../OrderBookV3FlashLender.reentrant.t.sol | 111 +++++++++++++++ test/util/concrete/Reenteroor.sol | 21 ++- 3 files changed, 199 insertions(+), 59 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 3ebe957f5..60bb5825e 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,62 +1,74 @@ -GenericPoolOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 333691, ~: 333142) -GenericPoolOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 267876, ~: 267870) -GenericPoolOrderBookV3FlashBorrowerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 612882, ~: 610958) -GenericPoolOrderBookV3FlashBorrowerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 525963, ~: 526185) -LibOrderTest:testHashEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 194389, ~: 191140) -LibOrderTest:testHashNotEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 298734, ~: 298554) -OrderBookAddOrderMockTest:testAddOrderSameAccountWithDifferentConfig(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,address) (runs: 5096, μ: 2771808, ~: 2761475) -OrderBookAddOrderMockTest:testAddOrderTwoAccountsWithDifferentConfig(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,address) (runs: 5096, μ: 2626597, ~: 2600883) -OrderBookAddOrderMockTest:testAddOrderTwoAccountsWithSameConfig(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2493242, ~: 2492266) -OrderBookAddOrderMockTest:testAddOrderWithCalculationsInputsAndOutputsSucceeds(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 1295793, ~: 1281958) -OrderBookAddOrderMockTest:testAddOrderWithNonEmptyMetaEmitsMetaV1(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 1303904, ~: 1290881) -OrderBookAddOrderMockTest:testAddOrderWithNonEmptyMetaReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 703690, ~: 697922) -OrderBookAddOrderMockTest:testAddOrderWithoutCalculationsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 226595, ~: 224802) -OrderBookAddOrderMockTest:testAddOrderWithoutInputsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 182418, ~: 181902) -OrderBookAddOrderMockTest:testAddOrderWithoutOutputsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 182840, ~: 182026) -OrderBookAddOrderTest:testAddOrderRealNoHandleIOReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 172931, ~: 171707) -OrderBookAddOrderTest:testAddOrderRealNoSourcesReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 176045, ~: 174413) -OrderBookAddOrderTest:testAddOrderRealOneStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 186224, ~: 185001) -OrderBookAddOrderTest:testAddOrderRealThreeStackCalculate(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 722003, ~: 718648) -OrderBookAddOrderTest:testAddOrderRealTwoStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 715298, ~: 711943) -OrderBookAddOrderTest:testAddOrderRealZeroStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 181518, ~: 180295) +GenericPoolOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332012, ~: 330416) +GenericPoolOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266191, ~: 265239) +GenericPoolOrderBookV3FlashBorrowerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 609047, ~: 605031) +GenericPoolOrderBookV3FlashBorrowerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 522441, ~: 519665) +LibOrderTest:testHashEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 196882, ~: 195948) +LibOrderTest:testHashNotEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 298108, ~: 295793) +OrderBookAddOrderMockTest:testAddOrderSameAccountWithDifferentConfig(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,address) (runs: 5096, μ: 2776550, ~: 2764101) +OrderBookAddOrderMockTest:testAddOrderTwoAccountsWithDifferentConfig(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,address) (runs: 5096, μ: 2598395, ~: 2573145) +OrderBookAddOrderMockTest:testAddOrderTwoAccountsWithSameConfig(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2495879, ~: 2483615) +OrderBookAddOrderMockTest:testAddOrderWithCalculationsInputsAndOutputsSucceeds(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 1295278, ~: 1291768) +OrderBookAddOrderMockTest:testAddOrderWithNonEmptyMetaEmitsMetaV1(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 1305903, ~: 1301693) +OrderBookAddOrderMockTest:testAddOrderWithNonEmptyMetaReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 702817, ~: 702452) +OrderBookAddOrderMockTest:testAddOrderWithoutCalculationsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 225204, ~: 222658) +OrderBookAddOrderMockTest:testAddOrderWithoutInputsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 180738, ~: 180602) +OrderBookAddOrderMockTest:testAddOrderWithoutOutputsReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 181522, ~: 180944) +OrderBookAddOrderTest:testAddOrderRealNoHandleIOReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 173437, ~: 171095) +OrderBookAddOrderTest:testAddOrderRealNoSourcesReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 176629, ~: 174639) +OrderBookAddOrderTest:testAddOrderRealOneStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 186730, ~: 184388) +OrderBookAddOrderTest:testAddOrderRealThreeStackCalculate(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 723949, ~: 717416) +OrderBookAddOrderTest:testAddOrderRealTwoStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 717244, ~: 710711) +OrderBookAddOrderTest:testAddOrderRealZeroStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 182024, ~: 179682) +OrderBookClearTest:testClearSimple(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,address,uint256,uint256) (runs: 5096, μ: 569599, ~: 569270) +OrderBookClearTest:testFlashLoanToNonReceiver(uint256,bytes,bytes32,bytes) (runs: 5096, μ: 28752, ~: 28661) OrderBookDepositTest:testDepositEvent(address,uint256,uint256) (runs: 5096, μ: 38710, ~: 38710) -OrderBookDepositTest:testDepositFail(address,uint256,uint256) (runs: 5096, μ: 8937393460516740789, ~: 8937393460516740786) +OrderBookDepositTest:testDepositFail(address,uint256,uint256) (runs: 5096, μ: 8937393460516740787, ~: 8937393460516740786) OrderBookDepositTest:testDepositGas00() (gas: 8176) -OrderBookDepositTest:testDepositGas01() (gas: 34620) -OrderBookDepositTest:testDepositMany((address,address,uint256,uint248)[]) (runs: 5096, μ: 4995652, ~: 4617129) -OrderBookDepositTest:testDepositOverflow(address,uint256,uint256,uint256) (runs: 5096, μ: 46645, ~: 46645) -OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 495335, ~: 496632) -OrderBookDepositTest:testDepositSimple(address,uint256,uint256) (runs: 5096, μ: 37840, ~: 37840) +OrderBookDepositTest:testDepositGas01() (gas: 34642) +OrderBookDepositTest:testDepositMany((address,address,uint256,uint248)[]) (runs: 5096, μ: 5225586, ~: 4912884) +OrderBookDepositTest:testDepositOverflow(address,uint256,uint256,uint256) (runs: 5096, μ: 46667, ~: 46667) +OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 596172, ~: 597676) +OrderBookDepositTest:testDepositSimple(address,uint256,uint256) (runs: 5096, μ: 37712, ~: 37712) OrderBookDepositTest:testDepositZero(address,uint256) (runs: 5096, μ: 12639, ~: 12639) -OrderBookDepositTest:testVaultBalanceNoDeposits(address,uint256) (runs: 5096, μ: 8263, ~: 8263) -OrderBookDepositTest:testVaultBalanceReentrant(address,uint256,uint256,address,address,uint256) (runs: 5096, μ: 494144, ~: 495964) -OrderBookRemoveOrderMockTest:testRemoveOrderAddRemoveMulti(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 7256622, ~: 7139425) -OrderBookRemoveOrderMockTest:testRemoveOrderDifferent(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 5070365, ~: 5046372) -OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwners(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 4854223, ~: 4841901) -OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwnersDifferent(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 10466985, ~: 10453491) -OrderBookRemoveOrderMockTest:testRemoveOrderDoesNotExist(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 386811, ~: 382063) -OrderBookRemoveOrderMockTest:testRemoveOrderOnlyOwner(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2603102, ~: 2601080) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrders(uint256,uint256) (runs: 5096, μ: 485915, ~: 503260) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrdersMultipleOwners(uint256,uint256,uint256) (runs: 5096, μ: 534417, ~: 560581) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleAnyDeposit(uint256,uint256) (runs: 5096, μ: 292768, ~: 298193) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumInput(uint256,uint256) (runs: 5096, μ: 278017, ~: 280317) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumOutput(uint256) (runs: 5096, μ: 278220, ~: 278155) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderUnlimitedMax(uint256) (runs: 5096, μ: 271824, ~: 271554) -OrderBookTakeOrderMaximumInputTest:testTakeOrderNoopZeroMaxTakerInput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,uint256[],bytes)) (runs: 5096, μ: 184268, ~: 183180) -OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderOne((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,uint256[],bytes)) (runs: 5096, μ: 431689, ~: 427938) -OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderTwo((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256,(address,uint256[],bytes),(address,uint256[],bytes)) (runs: 5096, μ: 851354, ~: 847183) -OrderBookTakeOrderNoopTest:testTakeOrderNoopZeroOrders() (gas: 12403) -OrderBookTakeOrderPrecisionTest:testTakeOrderPrecisionKnownBad01() (gas: 2631797) -OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 622309, ~: 616965) -OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 622709, ~: 616922) -OrderBookTakeOrderTokenMismatchTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 618189, ~: 618644) -OrderBookTakeOrderTokenMismatchTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 619539, ~: 619901) +OrderBookDepositTest:testVaultBalanceNoDeposits(address,uint256) (runs: 5096, μ: 6135, ~: 6135) +OrderBookRemoveOrderMockTest:testRemoveOrderAddRemoveMulti(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 7247993, ~: 7174461) +OrderBookRemoveOrderMockTest:testRemoveOrderDifferent(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 5046655, ~: 5004628) +OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwners(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 4860524, ~: 4822798) +OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwnersDifferent(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 10434905, ~: 10321656) +OrderBookRemoveOrderMockTest:testRemoveOrderDoesNotExist(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 386436, ~: 384448) +OrderBookRemoveOrderMockTest:testRemoveOrderOnlyOwner(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2606450, ~: 2590636) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert() (gas: 282024) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrders(uint256,uint256) (runs: 5096, μ: 483439, ~: 501086) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrdersMultipleOwners(uint256,uint256,uint256) (runs: 5096, μ: 532049, ~: 558239) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleAnyDeposit(uint256,uint256) (runs: 5096, μ: 290467, ~: 295936) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumInput(uint256,uint256) (runs: 5096, μ: 275733, ~: 278060) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumOutput(uint256) (runs: 5096, μ: 275945, ~: 275876) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderUnlimitedMax(uint256) (runs: 5096, μ: 269545, ~: 269275) +OrderBookTakeOrderMaximumInputTest:testTakeOrderNoopZeroMaxTakerInput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,uint256[],bytes)) (runs: 5096, μ: 185762, ~: 184115) +OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderOne((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,uint256[],bytes)) (runs: 5096, μ: 429836, ~: 428425) +OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderTwo((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256,(address,uint256[],bytes),(address,uint256[],bytes)) (runs: 5096, μ: 847256, ~: 844746) +OrderBookTakeOrderNoopTest:testTakeOrderNoopZeroOrders() (gas: 12358) +OrderBookTakeOrderPrecisionTest:testTakeOrderPrecisionKnownBad01() (gas: 2629544) +OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 619004, ~: 614892) +OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 620402, ~: 616161) +OrderBookTakeOrderTokenMismatchTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 618302, ~: 615901) +OrderBookTakeOrderTokenMismatchTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 619562, ~: 617164) +OrderBookV3FlashLenderFeeTest:testFlashFee(address,uint256) (runs: 5096, μ: 3702, ~: 3702) +OrderBookV3FlashLenderMaxFlashLoanTest:testFlashMaxLoan(uint256) (runs: 5096, μ: 7638, ~: 7638) +OrderBookV3FlashLenderMockSuccessTest:testFlashLoanToReceiver(uint256,bytes) (runs: 5096, μ: 14721, ~: 14674) +OrderBookV3FlashLenderReentrant:testReenterAddOrder(uint256,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 18612941, ~: 18460909) +OrderBookV3FlashLenderReentrant:testReenterCheckOrderExists(bytes32,uint256) (runs: 5096, μ: 552763, ~: 553611) +OrderBookV3FlashLenderReentrant:testReenterDeposit(uint256,uint256,uint256) (runs: 5096, μ: 628359, ~: 630008) +OrderBookV3FlashLenderReentrant:testReenterReadVaultBalances(uint256,uint256) (runs: 5096, μ: 597307, ~: 598916) +OrderBookV3FlashLenderReentrant:testReenterRemoveOrder(uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 17718676, ~: 17746711) +OrderBookV3FlashLenderReentrant:testReenterWithdraw(uint256,uint256,uint256) (runs: 5096, μ: 604785, ~: 606629) +OrderBookV3FlashLenderTransferTest:testFlashLoanTransferFail(uint256,uint256,bool) (runs: 5096, μ: 1344459, ~: 1344917) +OrderBookV3FlashLenderTransferTest:testFlashLoanTransferSuccess(uint256,bool) (runs: 5096, μ: 1288441, ~: 1297392) OrderBookWithdrawTest:testWithdrawEmptyVault(address,address,uint256,uint256) (runs: 5096, μ: 15251, ~: 15251) -OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516719287, ~: 8937393460516700418) -OrderBookWithdrawTest:testWithdrawFullVault(address,uint256,uint256,uint256) (runs: 5096, μ: 41257, ~: 41254) -OrderBookWithdrawTest:testWithdrawMany((bool,address,address,uint256,uint248)[]) (runs: 5096, μ: 2543106, ~: 2233585) -OrderBookWithdrawTest:testWithdrawPartialVault(address,uint256,uint256,uint256) (runs: 5096, μ: 51929, ~: 51929) -OrderBookWithdrawTest:testWithdrawReentrant(address,uint256,uint256,address,address,uint256) (runs: 5096, μ: 506122, ~: 507997) +OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516716998, ~: 8937393460516698169) +OrderBookWithdrawTest:testWithdrawFullVault(address,uint256,uint256,uint256) (runs: 5096, μ: 39264, ~: 39262) +OrderBookWithdrawTest:testWithdrawMany((bool,address,address,uint256,uint248)[]) (runs: 5096, μ: 3169906, ~: 3156574) +OrderBookWithdrawTest:testWithdrawPartialVault(address,uint256,uint256,uint256) (runs: 5096, μ: 49439, ~: 49439) OrderBookWithdrawTest:testWithdrawZero(address,address,uint256) (runs: 5096, μ: 12809, ~: 12809) -RouteProcessorOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 333852, ~: 333451) -RouteProcessorOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 268039, ~: 268033) \ No newline at end of file +RouteProcessorOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332178, ~: 330509) +RouteProcessorOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266351, ~: 265402) \ No newline at end of file diff --git a/test/concrete/OrderBookV3FlashLender.reentrant.t.sol b/test/concrete/OrderBookV3FlashLender.reentrant.t.sol index e69de29bb..c68477e55 100644 --- a/test/concrete/OrderBookV3FlashLender.reentrant.t.sol +++ b/test/concrete/OrderBookV3FlashLender.reentrant.t.sol @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {OrderBookExternalRealTest} from "test/util/abstract/OrderBookExternalRealTest.sol"; +import {Reenteroor} from "test/util/concrete/Reenteroor.sol"; +import {IOrderBookV3, OrderConfigV2, Order} from "src/interface/unstable/IOrderBookV3.sol"; +import {IParserV1} from "rain.interpreter/src/interface/unstable/IParserV1.sol"; +import {IERC3156FlashBorrower} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; +import {LibTestAddOrder} from "test/util/lib/LibTestAddOrder.sol"; + +/// @title OrderBookV3FlashLenderReentrant +/// Test that flash borrowers can reenter the orderbook, which is necessary for +/// trading etc. against it while the loan is active. +contract OrderBookV3FlashLenderReentrant is OrderBookExternalRealTest { + function checkFlashLoanNotRevert(Reenteroor borrower, bytes memory encodedCall, uint256 loanAmount) internal { + borrower.reenterWith(encodedCall); + vm.mockCall( + address(iToken0), + abi.encodeWithSelector(IERC20.transfer.selector, address(borrower), loanAmount), + abi.encode(true) + ); + vm.mockCall( + address(iToken0), + abi.encodeWithSelector(IERC20.approve.selector, address(iOrderbook), loanAmount), + abi.encode(true) + ); + vm.mockCall( + address(iToken0), + abi.encodeWithSelector(IERC20.transferFrom.selector, borrower, address(iOrderbook), loanAmount), + abi.encode(true) + ); + + // Create a flash loan. + iOrderbook.flashLoan(IERC3156FlashBorrower(address(borrower)), address(iToken0), loanAmount, ""); + } + + /// Can reenter and read vault balances from within a flash loan. + function testReenterReadVaultBalances(uint256 vaultId, uint256 loanAmount) external { + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + checkFlashLoanNotRevert( + borrower, + abi.encodeWithSelector(IOrderBookV3.vaultBalance.selector, address(borrower), address(iToken0), vaultId), + loanAmount + ); + } + + /// Can reenter and check if an order exists from within a flash loan. + function testReenterCheckOrderExists(bytes32 orderHash, uint256 loanAmount) external { + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + checkFlashLoanNotRevert( + borrower, abi.encodeWithSelector(IOrderBookV3.orderExists.selector, orderHash), loanAmount + ); + } + + /// Can reenter and deposit from within a flash loan. + function testReenterDeposit(uint256 vaultId, uint256 loanAmount, uint256 depositAmount) external { + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + depositAmount = bound(depositAmount, 1, type(uint256).max); + vm.mockCall( + address(iToken0), + abi.encodeWithSelector(IERC20.transferFrom.selector, borrower, address(iOrderbook), depositAmount), + abi.encode(true) + ); + checkFlashLoanNotRevert( + borrower, + abi.encodeWithSelector(IOrderBookV3.deposit.selector, address(iToken0), vaultId, depositAmount), + loanAmount + ); + } + + /// Can reenter and withdraw from within a flash loan. + function testReenterWithdraw(uint256 vaultId, uint256 loanAmount, uint256 withdrawAmount) external { + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + withdrawAmount = bound(withdrawAmount, 1, type(uint256).max); + vm.mockCall( + address(iToken0), + abi.encodeWithSelector(IERC20.transfer.selector, address(borrower), withdrawAmount), + abi.encode(true) + ); + checkFlashLoanNotRevert( + borrower, + abi.encodeWithSelector(IOrderBookV3.withdraw.selector, address(iToken0), vaultId, withdrawAmount), + loanAmount + ); + } + + /// Can reenter and add an order from within a flash loan. + function testReenterAddOrder(uint256 loanAmount, OrderConfigV2 memory config) external { + LibTestAddOrder.conformConfig(config, iDeployer); + (bytes memory bytecode, uint256[] memory constants) = + IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:;"); + config.evaluableConfig.bytecode = bytecode; + config.evaluableConfig.constants = constants; + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + checkFlashLoanNotRevert(borrower, abi.encodeWithSelector(IOrderBookV3.addOrder.selector, config), loanAmount); + } + + /// Can reenter and remove an order from within a flash loan. + function testReenterRemoveOrder(uint256 loanAmount, Order memory order) external { + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + order.owner = address(borrower); + checkFlashLoanNotRevert(borrower, abi.encodeWithSelector(IOrderBookV3.removeOrder.selector, order), loanAmount); + } +} diff --git a/test/util/concrete/Reenteroor.sol b/test/util/concrete/Reenteroor.sol index 4947426bd..95676b940 100644 --- a/test/util/concrete/Reenteroor.sol +++ b/test/util/concrete/Reenteroor.sol @@ -1,11 +1,16 @@ // SPDX-License-Identifier: CAL pragma solidity =0.8.19; -import "lib/openzeppelin-contracts/contracts/utils/Address.sol"; +import {Address} from "lib/openzeppelin-contracts/contracts/utils/Address.sol"; +import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; +import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; /// @title Reenteroor /// A contract that reenters the caller with a configurable call. -contract Reenteroor { +/// This is compatible with flash loans and can be used as a borrower that will +/// handle `onFlashLoan` and return success to the lender correctly, as well as +/// reentering. +contract Reenteroor is IERC3156FlashBorrower { using Address for address; /// The call to reenter with. Set by `reenterWith`. @@ -17,6 +22,18 @@ contract Reenteroor { _sEncodedCall = encodedCall; } + /// @inheritdoc IERC3156FlashBorrower + function onFlashLoan(address, address token, uint256 amount, uint256, bytes calldata) + external + override + returns (bytes32) + { + address(msg.sender).functionCall(_sEncodedCall); + // Approve the lender to pull the tokens back and repay the loan. + IERC20(token).approve(msg.sender, amount); + return ON_FLASH_LOAN_CALLBACK_SUCCESS; + } + /// Reenter the caller with the call set by `reenterWith`. This will bubble /// up any reverts from the reentrant call so tests can check that /// reentrancy guards are working. From 3726423d71c56d595cdb7bf5193a1ec92ef01d45 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 16:08:45 +0400 Subject: [PATCH 16/23] slither --- src/concrete/OrderBook.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 4a54d0356..e16e34a85 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -225,10 +225,6 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash {} /// @inheritdoc IOrderBookV3 - // This has a reentrancy guard on it but Slither doesn't know that because - // it is a read-only reentrancy due to potential cross function reentrancy. - // https://github.com/crytic/slither/issues/735#issuecomment-1620704314 - //slither-disable-next-line reentrancy-no-eth function vaultBalance(address owner, address token, uint256 vaultId) external view override returns (uint256) { return sVaultBalances[owner][token][vaultId]; } From b7c6bf0aa181dc7ea161016bb81d555dbffd24d9 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 16:11:42 +0400 Subject: [PATCH 17/23] docs --- src/concrete/OrderBook.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index e16e34a85..27b7d593c 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -306,6 +306,9 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash if (sOrders[orderHash] == ORDER_DEAD) { stateChanged = true; + // This has to come after the external call to deploy the expression + // because the order hash is derived from the expression and DISPair + // addresses. //slither-disable-next-line reentrancy-benign sOrders[orderHash] = ORDER_LIVE; emit AddOrder(msg.sender, config.evaluableConfig.deployer, order, orderHash); From a6d7d0a1e02677631f8e92f38cae84723166e3f4 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 10 Nov 2023 16:20:09 +0400 Subject: [PATCH 18/23] exclude solc version from slither --- slither.config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither.config.json b/slither.config.json index eefba0e32..9832991df 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,4 +1,4 @@ { - "detectors_to_exclude": "assembly-usage", + "detectors_to_exclude": "assembly-usage,solc-version", "filter_paths": "forge-std,openzeppelin,test" } From 2a36f96b889fde539d0ef878a12bbcf789f9b50e Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Mon, 13 Nov 2023 06:15:19 +0400 Subject: [PATCH 19/23] tests --- .gas-snapshot | 68 +++++++------ .../OrderBook.takeOrder.handleIO.revert.t.sol | 95 ++++++++++++++++--- .../OrderBookV3FlashLender.reentrant.t.sol | 90 +++++++++++++++++- 3 files changed, 207 insertions(+), 46 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 60bb5825e..1098e47b8 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,7 +1,7 @@ -GenericPoolOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332012, ~: 330416) -GenericPoolOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266191, ~: 265239) -GenericPoolOrderBookV3FlashBorrowerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 609047, ~: 605031) -GenericPoolOrderBookV3FlashBorrowerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 522441, ~: 519665) +GenericPoolOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332013, ~: 330346) +GenericPoolOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266192, ~: 265239) +GenericPoolOrderBookV3FlashBorrowerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 609043, ~: 605031) +GenericPoolOrderBookV3FlashBorrowerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 522442, ~: 519665) LibOrderTest:testHashEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 196882, ~: 195948) LibOrderTest:testHashNotEqual((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 298108, ~: 295793) OrderBookAddOrderMockTest:testAddOrderSameAccountWithDifferentConfig(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,address) (runs: 5096, μ: 2776550, ~: 2764101) @@ -22,53 +22,61 @@ OrderBookAddOrderTest:testAddOrderRealZeroStackCalculateReverts(address,((addres OrderBookClearTest:testClearSimple(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,address,uint256,uint256) (runs: 5096, μ: 569599, ~: 569270) OrderBookClearTest:testFlashLoanToNonReceiver(uint256,bytes,bytes32,bytes) (runs: 5096, μ: 28752, ~: 28661) OrderBookDepositTest:testDepositEvent(address,uint256,uint256) (runs: 5096, μ: 38710, ~: 38710) -OrderBookDepositTest:testDepositFail(address,uint256,uint256) (runs: 5096, μ: 8937393460516740787, ~: 8937393460516740786) +OrderBookDepositTest:testDepositFail(address,uint256,uint256) (runs: 5096, μ: 8937393460516740791, ~: 8937393460516740786) OrderBookDepositTest:testDepositGas00() (gas: 8176) OrderBookDepositTest:testDepositGas01() (gas: 34642) -OrderBookDepositTest:testDepositMany((address,address,uint256,uint248)[]) (runs: 5096, μ: 5225586, ~: 4912884) +OrderBookDepositTest:testDepositMany((address,address,uint256,uint248)[]) (runs: 5096, μ: 5241523, ~: 4958934) OrderBookDepositTest:testDepositOverflow(address,uint256,uint256,uint256) (runs: 5096, μ: 46667, ~: 46667) -OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 596172, ~: 597676) +OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 596098, ~: 597676) OrderBookDepositTest:testDepositSimple(address,uint256,uint256) (runs: 5096, μ: 37712, ~: 37712) OrderBookDepositTest:testDepositZero(address,uint256) (runs: 5096, μ: 12639, ~: 12639) OrderBookDepositTest:testVaultBalanceNoDeposits(address,uint256) (runs: 5096, μ: 6135, ~: 6135) OrderBookRemoveOrderMockTest:testRemoveOrderAddRemoveMulti(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 7247993, ~: 7174461) OrderBookRemoveOrderMockTest:testRemoveOrderDifferent(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 5046655, ~: 5004628) OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwners(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 4860524, ~: 4822798) -OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwnersDifferent(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 10434905, ~: 10321656) +OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwnersDifferent(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 10434451, ~: 10321656) OrderBookRemoveOrderMockTest:testRemoveOrderDoesNotExist(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 386436, ~: 384448) OrderBookRemoveOrderMockTest:testRemoveOrderOnlyOwner(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2606450, ~: 2590636) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert() (gas: 282024) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrders(uint256,uint256) (runs: 5096, μ: 483439, ~: 501086) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrdersMultipleOwners(uint256,uint256,uint256) (runs: 5096, μ: 532049, ~: 558239) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleAnyDeposit(uint256,uint256) (runs: 5096, μ: 290467, ~: 295936) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumInput(uint256,uint256) (runs: 5096, μ: 275733, ~: 278060) -OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumOutput(uint256) (runs: 5096, μ: 275945, ~: 275876) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert0() (gas: 282930) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert1() (gas: 495699) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert2() (gas: 495865) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert3() (gas: 690805) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert4() (gas: 708469) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert5() (gas: 708489) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert6() (gas: 708303) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrders(uint256,uint256) (runs: 5096, μ: 483323, ~: 501086) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrdersMultipleOwners(uint256,uint256,uint256) (runs: 5096, μ: 532058, ~: 558239) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleAnyDeposit(uint256,uint256) (runs: 5096, μ: 290502, ~: 295936) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumInput(uint256,uint256) (runs: 5096, μ: 275711, ~: 278060) +OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderLessThanMaximumOutput(uint256) (runs: 5096, μ: 275942, ~: 275876) OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleOrderUnlimitedMax(uint256) (runs: 5096, μ: 269545, ~: 269275) OrderBookTakeOrderMaximumInputTest:testTakeOrderNoopZeroMaxTakerInput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,uint256[],bytes)) (runs: 5096, μ: 185762, ~: 184115) -OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderOne((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,uint256[],bytes)) (runs: 5096, μ: 429836, ~: 428425) -OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderTwo((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256,(address,uint256[],bytes),(address,uint256[],bytes)) (runs: 5096, μ: 847256, ~: 844746) +OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderOne((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,uint256[],bytes)) (runs: 5096, μ: 429838, ~: 428425) +OrderBookTakeOrderNoopTest:testTakeOrderNoopNonLiveOrderTwo((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256,(address,uint256[],bytes),(address,uint256[],bytes)) (runs: 5096, μ: 847255, ~: 844850) OrderBookTakeOrderNoopTest:testTakeOrderNoopZeroOrders() (gas: 12358) OrderBookTakeOrderPrecisionTest:testTakeOrderPrecisionKnownBad01() (gas: 2629544) -OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 619004, ~: 614892) -OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 620402, ~: 616161) -OrderBookTakeOrderTokenMismatchTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 618302, ~: 615901) -OrderBookTakeOrderTokenMismatchTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 619562, ~: 617164) +OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 619265, ~: 615316) +OrderBookTakeOrderTokenMismatchDecimalsTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 619982, ~: 616076) +OrderBookTakeOrderTokenMismatchTest:testTokenMismatchInputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 618233, ~: 615901) +OrderBookTakeOrderTokenMismatchTest:testTokenMismatchOutputs((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 619563, ~: 617215) OrderBookV3FlashLenderFeeTest:testFlashFee(address,uint256) (runs: 5096, μ: 3702, ~: 3702) OrderBookV3FlashLenderMaxFlashLoanTest:testFlashMaxLoan(uint256) (runs: 5096, μ: 7638, ~: 7638) OrderBookV3FlashLenderMockSuccessTest:testFlashLoanToReceiver(uint256,bytes) (runs: 5096, μ: 14721, ~: 14674) -OrderBookV3FlashLenderReentrant:testReenterAddOrder(uint256,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 18612941, ~: 18460909) -OrderBookV3FlashLenderReentrant:testReenterCheckOrderExists(bytes32,uint256) (runs: 5096, μ: 552763, ~: 553611) -OrderBookV3FlashLenderReentrant:testReenterDeposit(uint256,uint256,uint256) (runs: 5096, μ: 628359, ~: 630008) -OrderBookV3FlashLenderReentrant:testReenterReadVaultBalances(uint256,uint256) (runs: 5096, μ: 597307, ~: 598916) -OrderBookV3FlashLenderReentrant:testReenterRemoveOrder(uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 17718676, ~: 17746711) -OrderBookV3FlashLenderReentrant:testReenterWithdraw(uint256,uint256,uint256) (runs: 5096, μ: 604785, ~: 606629) -OrderBookV3FlashLenderTransferTest:testFlashLoanTransferFail(uint256,uint256,bool) (runs: 5096, μ: 1344459, ~: 1344917) +OrderBookV3FlashLenderReentrant:testReenterAddOrder(uint256,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 18621561, ~: 18497524) +OrderBookV3FlashLenderReentrant:testReenterCheckOrderExists(bytes32,uint256) (runs: 5096, μ: 552771, ~: 553611) +OrderBookV3FlashLenderReentrant:testReenterClear(uint256,address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 37709735, ~: 37616965) +OrderBookV3FlashLenderReentrant:testReenterDeposit(uint256,uint256,uint256) (runs: 5096, μ: 628333, ~: 630052) +OrderBookV3FlashLenderReentrant:testReenterReadVaultBalances(uint256,uint256) (runs: 5096, μ: 597383, ~: 598938) +OrderBookV3FlashLenderReentrant:testReenterRemoveOrder(uint256,(address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[])) (runs: 5096, μ: 17734084, ~: 17708028) +OrderBookV3FlashLenderReentrant:testReenterTakeOrder(uint256,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 19224537, ~: 19063930) +OrderBookV3FlashLenderReentrant:testReenterWithdraw(uint256,uint256,uint256) (runs: 5096, μ: 604691, ~: 606629) +OrderBookV3FlashLenderTransferTest:testFlashLoanTransferFail(uint256,uint256,bool) (runs: 5096, μ: 1344450, ~: 1344917) OrderBookV3FlashLenderTransferTest:testFlashLoanTransferSuccess(uint256,bool) (runs: 5096, μ: 1288441, ~: 1297392) OrderBookWithdrawTest:testWithdrawEmptyVault(address,address,uint256,uint256) (runs: 5096, μ: 15251, ~: 15251) -OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516716998, ~: 8937393460516698169) +OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516716964, ~: 8937393460516698169) OrderBookWithdrawTest:testWithdrawFullVault(address,uint256,uint256,uint256) (runs: 5096, μ: 39264, ~: 39262) OrderBookWithdrawTest:testWithdrawMany((bool,address,address,uint256,uint248)[]) (runs: 5096, μ: 3169906, ~: 3156574) OrderBookWithdrawTest:testWithdrawPartialVault(address,uint256,uint256,uint256) (runs: 5096, μ: 49439, ~: 49439) OrderBookWithdrawTest:testWithdrawZero(address,address,uint256) (runs: 5096, μ: 12809, ~: 12809) -RouteProcessorOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332178, ~: 330509) -RouteProcessorOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266351, ~: 265402) \ No newline at end of file +RouteProcessorOrderBookV3ArbOrderTakerTest:testMinimumOutput((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256,uint256,uint256) (runs: 5096, μ: 332180, ~: 330641) +RouteProcessorOrderBookV3ArbOrderTakerTest:testTakeOrdersSender((address,bool,(address,address,address),(address,uint8,uint256)[],(address,uint8,uint256)[]),uint256,uint256) (runs: 5096, μ: 266353, ~: 265402) \ No newline at end of file diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol index 001acba27..a4f7edb9a 100644 --- a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -19,20 +19,19 @@ import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol" /// @notice A test harness for testing the OrderBook takeOrder function will run /// handle IO and revert if it fails. contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { - function testTakeOrderHandleIORevert() public { + function checkTakeOrderHandleIORevert(bytes[] memory configs, bytes memory err) public { uint256 vaultId = 0; address inputToken = address(0x100); address outputToken = address(0x101); + OrderConfigV2 memory config; + IO[] memory validOutputs; + IO[] memory validInputs; { - IO[] memory validInputs = new IO[](1); + validInputs = new IO[](1); validInputs[0] = IO(inputToken, 18, vaultId); - IO[] memory validOutputs = new IO[](1); + validOutputs = new IO[](1); validOutputs[0] = IO(outputToken, 18, vaultId); - (bytes memory bytecode, uint256[] memory constants) = - IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:ensure<1>(0);"); - EvaluableConfigV2 memory evaluableConfig = EvaluableConfigV2(iDeployer, bytecode, constants); - config = OrderConfigV2(validInputs, validOutputs, evaluableConfig, ""); // Etch with invalid. vm.etch(outputToken, hex"fe"); vm.etch(inputToken, hex"fe"); @@ -44,19 +43,85 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { iOrderbook.deposit(outputToken, vaultId, type(uint256).max); assertEq(iOrderbook.vaultBalance(address(this), outputToken, vaultId), type(uint256).max); - vm.recordLogs(); - iOrderbook.addOrder(config); - Vm.Log[] memory entries = vm.getRecordedLogs(); - assertEq(entries.length, 3); - (,, Order memory order,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + TakeOrderConfig[] memory orders = new TakeOrderConfig[](configs.length); + + for (uint256 i = 0; i < configs.length; i++) { + (bytes memory bytecode, uint256[] memory constants) = IParserV1(address(iDeployer)).parse(configs[i]); + EvaluableConfigV2 memory evaluableConfig = EvaluableConfigV2(iDeployer, bytecode, constants); + config = OrderConfigV2(validInputs, validOutputs, evaluableConfig, ""); - TakeOrderConfig[] memory orders = new TakeOrderConfig[](1); - orders[0] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); + vm.recordLogs(); + iOrderbook.addOrder(config); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 3); + (,, Order memory order,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + orders[i] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); + } TakeOrdersConfigV2 memory takeOrdersConfig = TakeOrdersConfigV2(0, type(uint256).max, type(uint256).max, orders, ""); - vm.expectRevert(abi.encodeWithSelector(EnsureFailed.selector, 1, 0)); + vm.expectRevert(err); (uint256 totalTakerInput, uint256 totalTakerOutput) = iOrderbook.takeOrders(takeOrdersConfig); (totalTakerInput, totalTakerOutput); } + + function testTakeOrderHandleIORevert0() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes[] memory configs = new bytes[](1); + configs[0] = "_ _:max-int-value() 1e18;:ensure<1>(0);"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert1() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes[] memory configs = new bytes[](2); + configs[0] = "_ _:1e18 1e18;:ensure<1>(0);"; + configs[1] = "_ _:1e18 1e18;:;"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert2() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes[] memory configs = new bytes[](2); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert3() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes[] memory configs = new bytes[](3); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; + configs[2] = "_ _:1e18 1e18;:;"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert4() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes[] memory configs = new bytes[](3); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; + configs[2] = "_ _:1e18 1e18;:ensure<2>(0);"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert5() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + bytes[] memory configs = new bytes[](3); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:ensure<2>(0);"; + configs[2] = "_ _:1e18 1e18;:ensure<1>(0);"; + checkTakeOrderHandleIORevert(configs, err); + } + + function testTakeOrderHandleIORevert6() external { + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + bytes[] memory configs = new bytes[](3); + configs[0] = "_ _:1e18 1e18;:ensure<2>(0);"; + configs[1] = "_ _:1e18 1e18;:;"; + configs[2] = "_ _:1e18 1e18;:ensure<1>(0);"; + checkTakeOrderHandleIORevert(configs, err); + } } diff --git a/test/concrete/OrderBookV3FlashLender.reentrant.t.sol b/test/concrete/OrderBookV3FlashLender.reentrant.t.sol index c68477e55..4132e7470 100644 --- a/test/concrete/OrderBookV3FlashLender.reentrant.t.sol +++ b/test/concrete/OrderBookV3FlashLender.reentrant.t.sol @@ -1,13 +1,22 @@ // SPDX-License-Identifier: CAL pragma solidity =0.8.19; +import {Vm} from "forge-std/Vm.sol"; import {OrderBookExternalRealTest} from "test/util/abstract/OrderBookExternalRealTest.sol"; import {Reenteroor} from "test/util/concrete/Reenteroor.sol"; -import {IOrderBookV3, OrderConfigV2, Order} from "src/interface/unstable/IOrderBookV3.sol"; +import { + IOrderBookV3, + OrderConfigV2, + Order, + TakeOrderConfig, + TakeOrdersConfigV2, + ClearConfig +} from "src/interface/unstable/IOrderBookV3.sol"; import {IParserV1} from "rain.interpreter/src/interface/unstable/IParserV1.sol"; import {IERC3156FlashBorrower} from "src/interface/ierc3156/IERC3156FlashBorrower.sol"; import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; import {LibTestAddOrder} from "test/util/lib/LibTestAddOrder.sol"; +import {SignedContextV1} from "rain.interpreter/src/interface/IInterpreterCallerV2.sol"; /// @title OrderBookV3FlashLenderReentrant /// Test that flash borrowers can reenter the orderbook, which is necessary for @@ -108,4 +117,83 @@ contract OrderBookV3FlashLenderReentrant is OrderBookExternalRealTest { order.owner = address(borrower); checkFlashLoanNotRevert(borrower, abi.encodeWithSelector(IOrderBookV3.removeOrder.selector, order), loanAmount); } + + /// Can reenter and take orders. + function testReenterTakeOrder(uint256 loanAmount, OrderConfigV2 memory config) external { + LibTestAddOrder.conformConfig(config, iDeployer); + (bytes memory bytecode, uint256[] memory constants) = + IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:;"); + config.evaluableConfig.bytecode = bytecode; + config.evaluableConfig.constants = constants; + + vm.recordLogs(); + iOrderbook.addOrder(config); + Vm.Log[] memory entries = vm.getRecordedLogs(); + (,, Order memory order,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + TakeOrderConfig[] memory orders = new TakeOrderConfig[](1); + orders[0] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); + TakeOrdersConfigV2 memory takeOrdersConfig = + TakeOrdersConfigV2(0, type(uint256).max, type(uint256).max, orders, ""); + + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + checkFlashLoanNotRevert( + borrower, abi.encodeWithSelector(IOrderBookV3.takeOrders.selector, takeOrdersConfig), loanAmount + ); + } + + /// Can reenter and clear orders. + function testReenterClear( + uint256 loanAmount, + address alice, + address bob, + OrderConfigV2 memory aliceConfig, + OrderConfigV2 memory bobConfig + ) external { + vm.assume(alice != bob); + + LibTestAddOrder.conformConfig(aliceConfig, iDeployer); + (bytes memory bytecode, uint256[] memory constants) = + IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:;"); + aliceConfig.evaluableConfig.bytecode = bytecode; + aliceConfig.evaluableConfig.constants = constants; + + LibTestAddOrder.conformConfig(bobConfig, iDeployer); + (bytecode, constants) = IParserV1(address(iDeployer)).parse("_ _:max-int-value() 1e18;:;"); + bobConfig.evaluableConfig.bytecode = bytecode; + bobConfig.evaluableConfig.constants = constants; + + bobConfig.validInputs[0] = aliceConfig.validOutputs[0]; + aliceConfig.validInputs[0] = bobConfig.validOutputs[0]; + + vm.recordLogs(); + vm.prank(alice); + iOrderbook.addOrder(aliceConfig); + Vm.Log[] memory entries = vm.getRecordedLogs(); + (,, Order memory aliceOrder,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + vm.recordLogs(); + vm.prank(bob); + iOrderbook.addOrder(bobConfig); + entries = vm.getRecordedLogs(); + (,, Order memory bobOrder,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + ClearConfig memory clearConfig = ClearConfig(0, 0, 0, 0, 0, 0); + + // Create a flash borrower. + Reenteroor borrower = new Reenteroor(); + checkFlashLoanNotRevert( + borrower, + abi.encodeWithSelector( + IOrderBookV3.clear.selector, + aliceOrder, + bobOrder, + clearConfig, + new SignedContextV1[](0), + new SignedContextV1[](0) + ), + loanAmount + ); + } } From 748accdd133ac1ebb5ca8a67e9b47ff7d766f937 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Tue, 14 Nov 2023 01:26:22 +0400 Subject: [PATCH 20/23] lint --- src/concrete/OrderBook.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 27b7d593c..cd67f0e42 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -477,7 +477,9 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash emit TakeOrder(msg.sender, takeOrderConfig, takerInput, takerOutput); // Add the pointer to the order IO calculation to the array - // of order IO calculations to handle. + // of order IO calculations to handle. This is + // unconditional because conditional behaviour is checked + // in `handleIO` and we don't want to duplicate that. assembly ("memory-safe") { // Inc the length by 1. let newLength := add(mload(orderIOCalculationsToHandle), 1) From a0d6cd24c1fa1a50030e5d39c5c3589ef399dacd Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 17 Nov 2023 16:16:24 +0400 Subject: [PATCH 21/23] more tests --- .../OrderBook.takeOrder.handleIO.revert.t.sol | 65 ++++++++++++++----- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol index a4f7edb9a..a20f68c38 100644 --- a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -19,7 +19,7 @@ import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol" /// @notice A test harness for testing the OrderBook takeOrder function will run /// handle IO and revert if it fails. contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { - function checkTakeOrderHandleIORevert(bytes[] memory configs, bytes memory err) public { + function checkTakeOrderHandleIO(bytes[] memory configs, bytes memory err, uint256 maxInput) public { uint256 vaultId = 0; address inputToken = address(0x100); address outputToken = address(0x101); @@ -59,69 +59,100 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { orders[i] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); } TakeOrdersConfigV2 memory takeOrdersConfig = - TakeOrdersConfigV2(0, type(uint256).max, type(uint256).max, orders, ""); + TakeOrdersConfigV2(0, maxInput, type(uint256).max, orders, ""); - vm.expectRevert(err); + if (err.length > 0) { + vm.expectRevert(err); + } (uint256 totalTakerInput, uint256 totalTakerOutput) = iOrderbook.takeOrders(takeOrdersConfig); + // We don't really care about the outputs as the tests are basically just + // trying to show that the IO handler is running or not running by simple + // reverts. (totalTakerInput, totalTakerOutput); } - function testTakeOrderHandleIORevert0() external { + function testTakeOrderHandleIO0() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); bytes[] memory configs = new bytes[](1); configs[0] = "_ _:max-int-value() 1e18;:ensure<1>(0);"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert1() external { + function testTakeOrderHandleIO1() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); bytes[] memory configs = new bytes[](2); configs[0] = "_ _:1e18 1e18;:ensure<1>(0);"; configs[1] = "_ _:1e18 1e18;:;"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert2() external { + function testTakeOrderHandleIO2() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); bytes[] memory configs = new bytes[](2); configs[0] = "_ _:1e18 1e18;:;"; configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert3() external { + function testTakeOrderHandleIO3() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); bytes[] memory configs = new bytes[](3); configs[0] = "_ _:1e18 1e18;:;"; configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; configs[2] = "_ _:1e18 1e18;:;"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert4() external { + function testTakeOrderHandleIO4() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); bytes[] memory configs = new bytes[](3); configs[0] = "_ _:1e18 1e18;:;"; configs[1] = "_ _:1e18 1e18;:ensure<1>(0);"; configs[2] = "_ _:1e18 1e18;:ensure<2>(0);"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert5() external { + function testTakeOrderHandleIO5() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); bytes[] memory configs = new bytes[](3); configs[0] = "_ _:1e18 1e18;:;"; configs[1] = "_ _:1e18 1e18;:ensure<2>(0);"; configs[2] = "_ _:1e18 1e18;:ensure<1>(0);"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); } - function testTakeOrderHandleIORevert6() external { + function testTakeOrderHandleIO6() external { bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); bytes[] memory configs = new bytes[](3); configs[0] = "_ _:1e18 1e18;:ensure<2>(0);"; configs[1] = "_ _:1e18 1e18;:;"; configs[2] = "_ _:1e18 1e18;:ensure<1>(0);"; - checkTakeOrderHandleIORevert(configs, err); + checkTakeOrderHandleIO(configs, err, type(uint256).max); + } + + function testTakeOrderHandleIO7(uint256 toClear) external { + toClear = bound(toClear, 4e18 + 1, type(uint256).max); + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + bytes[] memory configs = new bytes[](5); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:set(0 1);"; + configs[2] = "_ _:1e18 1e18;:ensure<1>(get(0));"; + configs[3] = "_ _:1e18 1e18;:set(0 0);"; + configs[4] = "_ _:1e18 1e18;:ensure<2>(get(0));"; + checkTakeOrderHandleIO(configs, err, toClear); + } + + // This one WONT error because the take orders stops executing the handle IO + // before it clears 4e18 + 1, so it never hits the second ensure condition. + function testTakeOrderHandleIO8(uint256 toClear) external { + toClear = bound(toClear, 1, 4e18); + bytes memory err = ""; + bytes[] memory configs = new bytes[](5); + configs[0] = "_ _:1e18 1e18;:;"; + configs[1] = "_ _:1e18 1e18;:set(0 1);"; + configs[2] = "_ _:1e18 1e18;:ensure<1>(get(0));"; + configs[3] = "_ _:1e18 1e18;:set(0 0);"; + configs[4] = "_ _:1e18 1e18;:ensure<2>(get(0));"; + checkTakeOrderHandleIO(configs, err, toClear); } } From 0b51c8f7b2622f03da972ed3b60cbd3391a27b4c Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 17 Nov 2023 17:03:49 +0400 Subject: [PATCH 22/23] more tests --- src/concrete/OrderBook.sol | 9 ++++--- .../OrderBook.takeOrder.handleIO.revert.t.sol | 26 ++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index cd67f0e42..c5dc6b4db 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -351,9 +351,12 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash TakeOrderConfig memory takeOrderConfig; Order memory order; - // Allocate a region of pointers but don't initialize it. Not even with - // the length. We'll do that later when we know how many orders we're - // actually going to handle. + // Allocate a region of memory to hold pointers. We don't know how many + // will run at this point, but we conservatively set aside a slot for + // every order in case we need it, rather than attempt to dynamically + // resize the array later. There's no guarantee that a dynamic solution + // would even be cheaper gas-wise, and it would almost certainly be more + // complex. OrderIOCalculation[] memory orderIOCalculationsToHandle; { uint256 length = config.orders.length; diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol index a20f68c38..397cbdc7e 100644 --- a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -131,6 +131,17 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { } function testTakeOrderHandleIO7(uint256 toClear) external { + toClear = bound(toClear, 3e18 + 1, type(uint256).max); + bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + bytes[] memory configs = new bytes[](4); + configs[0] = "_ _:1e18 1e18;:set(0 1);"; + configs[1] = "_ _:1e18 1e18;:ensure<1>(get(0));"; + configs[2] = "_ _:1e18 1e18;:set(0 0);"; + configs[3] = "_ _:1e18 1e18;:ensure<2>(get(0));"; + checkTakeOrderHandleIO(configs, err, toClear); + } + + function testTakeOrderHandleIO8(uint256 toClear) external { toClear = bound(toClear, 4e18 + 1, type(uint256).max); bytes memory err = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); bytes[] memory configs = new bytes[](5); @@ -144,7 +155,7 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { // This one WONT error because the take orders stops executing the handle IO // before it clears 4e18 + 1, so it never hits the second ensure condition. - function testTakeOrderHandleIO8(uint256 toClear) external { + function testTakeOrderHandleIO9(uint256 toClear) external { toClear = bound(toClear, 1, 4e18); bytes memory err = ""; bytes[] memory configs = new bytes[](5); @@ -155,4 +166,17 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { configs[4] = "_ _:1e18 1e18;:ensure<2>(get(0));"; checkTakeOrderHandleIO(configs, err, toClear); } + + // This one WONT error because the take orders stops executing the handle IO + // before it clears 4e18 + 1, so it never hits the second ensure condition. + function testTakeOrderHandleIO10(uint256 toClear) external { + toClear = bound(toClear, 1, 3e18); + bytes memory err = ""; + bytes[] memory configs = new bytes[](4); + configs[0] = "_ _:1e18 1e18;:set(0 1);"; + configs[1] = "_ _:1e18 1e18;:ensure<1>(get(0));"; + configs[2] = "_ _:1e18 1e18;:set(0 0);"; + configs[3] = "_ _:1e18 1e18;:ensure<2>(get(0));"; + checkTakeOrderHandleIO(configs, err, toClear); + } } From f3541ea3a5fde3ad7a942db6556301e2a6524e75 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Sat, 18 Nov 2023 00:06:14 +0400 Subject: [PATCH 23/23] more tests --- .gas-snapshot | 24 ++- .../OrderBook.clear.handleIO.revert.t.sol | 147 ++++++++++++++++++ .../OrderBook.takeOrder.handleIO.revert.t.sol | 5 +- 3 files changed, 166 insertions(+), 10 deletions(-) create mode 100644 test/concrete/OrderBook.clear.handleIO.revert.t.sol diff --git a/.gas-snapshot b/.gas-snapshot index 1098e47b8..596b166e8 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -19,6 +19,12 @@ OrderBookAddOrderTest:testAddOrderRealOneStackCalculateReverts(address,((address OrderBookAddOrderTest:testAddOrderRealThreeStackCalculate(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 723949, ~: 717416) OrderBookAddOrderTest:testAddOrderRealTwoStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 717244, ~: 710711) OrderBookAddOrderTest:testAddOrderRealZeroStackCalculateReverts(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes)) (runs: 5096, μ: 182024, ~: 179682) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO0() (gas: 629944) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO1() (gas: 612268) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO2() (gas: 612253) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO3() (gas: 629982) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO4() (gas: 642423) +OrderBookClearHandleIORevertTest:testClearOrderHandleIO5() (gas: 555978) OrderBookClearTest:testClearSimple(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),uint256,address,address,uint256,uint256) (runs: 5096, μ: 569599, ~: 569270) OrderBookClearTest:testFlashLoanToNonReceiver(uint256,bytes,bytes32,bytes) (runs: 5096, μ: 28752, ~: 28661) OrderBookDepositTest:testDepositEvent(address,uint256,uint256) (runs: 5096, μ: 38710, ~: 38710) @@ -37,13 +43,17 @@ OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwners(address,address,((ad OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwnersDifferent(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 10434451, ~: 10321656) OrderBookRemoveOrderMockTest:testRemoveOrderDoesNotExist(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 386436, ~: 384448) OrderBookRemoveOrderMockTest:testRemoveOrderOnlyOwner(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2606450, ~: 2590636) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert0() (gas: 282930) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert1() (gas: 495699) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert2() (gas: 495865) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert3() (gas: 690805) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert4() (gas: 708469) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert5() (gas: 708489) -OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIORevert6() (gas: 708303) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO0() (gas: 282916) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO1() (gas: 495749) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO10(uint256) (runs: 5096, μ: 878430, ~: 836750) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO2() (gas: 495915) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO3() (gas: 690855) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO4() (gas: 708540) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO5() (gas: 708561) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO6() (gas: 708353) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO7(uint256) (runs: 5096, μ: 1022470, ~: 1022199) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO8(uint256) (runs: 5096, μ: 1217981, ~: 1217713) +OrderBookTakeOrderHandleIORevertTest:testTakeOrderHandleIO9(uint256) (runs: 5096, μ: 1020159, ~: 939210) OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrders(uint256,uint256) (runs: 5096, μ: 483323, ~: 501086) OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputMultipleOrdersMultipleOwners(uint256,uint256,uint256) (runs: 5096, μ: 532058, ~: 558239) OrderBookTakeOrderMaximumInputTest:testTakeOrderMaximumInputSingleAnyDeposit(uint256,uint256) (runs: 5096, μ: 290502, ~: 295936) diff --git a/test/concrete/OrderBook.clear.handleIO.revert.t.sol b/test/concrete/OrderBook.clear.handleIO.revert.t.sol new file mode 100644 index 000000000..4c10cec41 --- /dev/null +++ b/test/concrete/OrderBook.clear.handleIO.revert.t.sol @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import {Vm} from "forge-std/Vm.sol"; +import { + OrderBookExternalRealTest, + OrderConfigV2, + IO, + TakeOrderConfig, + Order, + IParserV1, + EvaluableConfigV2, + ClearConfig, + SignedContextV1 +} from "test/util/abstract/OrderBookExternalRealTest.sol"; +import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol"; + +/// @title OrderBookClearHandleIORevertTest +/// @notice A test harness for testing the OrderBook clear function will run +/// handle IO and revert if it fails. +contract OrderBookClearHandleIORevertTest is OrderBookExternalRealTest { + function userDeposit(bytes memory rainString, address owner, address inputToken, address outputToken) + internal + returns (Order memory) + { + uint256 vaultId = 0; + + OrderConfigV2 memory config; + IO[] memory validOutputs; + IO[] memory validInputs; + { + validInputs = new IO[](1); + validInputs[0] = IO(inputToken, 18, vaultId); + validOutputs = new IO[](1); + validOutputs[0] = IO(outputToken, 18, vaultId); + // Etch with invalid. + vm.etch(inputToken, hex"fe"); + vm.etch(outputToken, hex"fe"); + // Mock every call to output as a success, so the orderbook thinks it + // is transferring tokens. + vm.mockCall(inputToken, "", abi.encode(true)); + vm.mockCall(outputToken, "", abi.encode(true)); + } + + vm.prank(owner); + iOrderbook.deposit(outputToken, vaultId, type(uint256).max); + assertEq(iOrderbook.vaultBalance(owner, outputToken, vaultId), type(uint256).max); + + (bytes memory bytecode, uint256[] memory constants) = IParserV1(address(iDeployer)).parse(rainString); + EvaluableConfigV2 memory evaluableConfig = EvaluableConfigV2(iDeployer, bytecode, constants); + config = OrderConfigV2(validInputs, validOutputs, evaluableConfig, ""); + + vm.prank(owner); + vm.recordLogs(); + iOrderbook.addOrder(config); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 3); + (,, Order memory order,) = abi.decode(entries[2].data, (address, address, Order, bytes32)); + + return order; + } + + function checkClearOrderHandleIO( + bytes memory aliceString, + bytes memory bobString, + bytes memory aliceErr, + bytes memory bobErr + ) internal { + address aliceInputToken = address(0x100); + address aliceOutputToken = address(0x101); + address alice = address(0x102); + address bob = address(0x103); + + Order memory aliceOrder = userDeposit(aliceString, alice, aliceInputToken, aliceOutputToken); + Order memory bobOrder = userDeposit(bobString, bob, aliceOutputToken, aliceInputToken); + ClearConfig memory clearConfig = ClearConfig(0, 0, 0, 0, 0, 0); + if (aliceErr.length > 0) { + vm.expectRevert(aliceErr); + } + iOrderbook.clear(aliceOrder, bobOrder, clearConfig, new SignedContextV1[](0), new SignedContextV1[](0)); + + if (bobErr.length > 0) { + vm.expectRevert(bobErr); + } + iOrderbook.clear(bobOrder, aliceOrder, clearConfig, new SignedContextV1[](0), new SignedContextV1[](0)); + } + + function testClearOrderHandleIO0() external { + bytes memory aliceErr = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes memory bobErr = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + + bytes memory aliceString = "_ _:max-int-value() 1e18;:ensure<1>(0);"; + bytes memory bobString = "_ _:max-int-value() 1e18;:ensure<2>(0);"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } + + function testClearOrderHandleIO1() external { + bytes memory aliceErr = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + bytes memory bobErr = abi.encodeWithSelector(EnsureFailed.selector, 2, 0); + + bytes memory aliceString = "_ _:max-int-value() 1e18;:;"; + bytes memory bobString = "_ _:max-int-value() 1e18;:ensure<2>(0);"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } + + function testClearOrderHandleIO2() external { + bytes memory aliceErr = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes memory bobErr = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + + bytes memory aliceString = "_ _:max-int-value() 1e18;:ensure<1>(0);"; + bytes memory bobString = "_ _:max-int-value() 1e18;:;"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } + + function testClearOrderHandleIO3() external { + bytes memory aliceErr = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + bytes memory bobErr = abi.encodeWithSelector(EnsureFailed.selector, 1, 0); + + bytes memory aliceString = "_ _:max-int-value() 1e18;:ensure<1>(0);"; + bytes memory bobString = "_ _:max-int-value() 1e18;:ensure<1>(0);"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } + + function testClearOrderHandleIO4() external { + bytes memory aliceErr = ""; + bytes memory bobErr = ""; + + bytes memory aliceString = "_ _:max-int-value() 1e18;:ensure<1>(1);"; + bytes memory bobString = "_ _:max-int-value() 1e18;:ensure<1>(1);"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } + + function testClearOrderHandleIO5() external { + bytes memory aliceErr = ""; + bytes memory bobErr = ""; + + bytes memory aliceString = "_ _:max-int-value() 1e18;:;"; + bytes memory bobString = "_ _:max-int-value() 1e18;:;"; + + checkClearOrderHandleIO(aliceString, bobString, aliceErr, bobErr); + } +} diff --git a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol index 397cbdc7e..29ccbf434 100644 --- a/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol +++ b/test/concrete/OrderBook.takeOrder.handleIO.revert.t.sol @@ -19,7 +19,7 @@ import {EnsureFailed} from "rain.interpreter/src/lib/op/logic/LibOpEnsureNP.sol" /// @notice A test harness for testing the OrderBook takeOrder function will run /// handle IO and revert if it fails. contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { - function checkTakeOrderHandleIO(bytes[] memory configs, bytes memory err, uint256 maxInput) public { + function checkTakeOrderHandleIO(bytes[] memory configs, bytes memory err, uint256 maxInput) internal { uint256 vaultId = 0; address inputToken = address(0x100); address outputToken = address(0x101); @@ -58,8 +58,7 @@ contract OrderBookTakeOrderHandleIORevertTest is OrderBookExternalRealTest { orders[i] = TakeOrderConfig(order, 0, 0, new SignedContextV1[](0)); } - TakeOrdersConfigV2 memory takeOrdersConfig = - TakeOrdersConfigV2(0, maxInput, type(uint256).max, orders, ""); + TakeOrdersConfigV2 memory takeOrdersConfig = TakeOrdersConfigV2(0, maxInput, type(uint256).max, orders, ""); if (err.length > 0) { vm.expectRevert(err);