diff --git a/.gas-snapshot b/.gas-snapshot index bcac5305d..6bf2b7c5d 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -3,7 +3,7 @@ GenericPoolOrderBookFlashBorrowerTest:testTakeOrdersSender() (gas: 2241348) 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, μ: 2626613, ~: 2600883) +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, μ: 2626417, ~: 2600441) OrderBookAddOrderMockTest:testAddOrderTwoAccountsWithSameConfig(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2493275, ~: 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, μ: 1303882, ~: 1290859) @@ -21,23 +21,24 @@ OrderBookDepositTest:testDepositEvent(address,uint256,uint256) (runs: 5096, μ: 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, μ: 5045831, ~: 4663301) +OrderBookDepositTest:testDepositMany((address,address,uint256,uint248)[]) (runs: 5096, μ: 5110729, ~: 4755733) OrderBookDepositTest:testDepositOverflow(address,uint256,uint256,uint256) (runs: 5096, μ: 46645, ~: 46645) -OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 495256, ~: 496632) +OrderBookDepositTest:testDepositReentrancy(address,uint256,uint256,address,uint256,uint256) (runs: 5096, μ: 495105, ~: 496632) OrderBookDepositTest:testDepositSimple(address,uint256,uint256) (runs: 5096, μ: 37840, ~: 37840) 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, μ: 494105, ~: 495964) +OrderBookDepositTest:testVaultBalanceReentrant(address,uint256,uint256,address,address,uint256) (runs: 5096, μ: 494249, ~: 495964) OrderBookRemoveOrderMockTest:testRemoveOrderAddRemoveMulti(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 7256892, ~: 7139695) 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, μ: 5070545, ~: 5046552) OrderBookRemoveOrderMockTest:testRemoveOrderDifferentOwners(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 4854493, ~: 4842171) -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, μ: 10468846, ~: 10456300) +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, μ: 10467503, ~: 10454009) OrderBookRemoveOrderMockTest:testRemoveOrderDoesNotExist(address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 386856, ~: 382108) OrderBookRemoveOrderMockTest:testRemoveOrderOnlyOwner(address,address,((address,uint8,uint256)[],(address,uint8,uint256)[],(address,bytes,uint256[]),bytes),address) (runs: 5096, μ: 2603327, ~: 2601305) +OrderBookTakeOrderTest:testTakeOrderPrecisionKnownBad01() (gas: 2556867) OrderBookWithdrawTest:testWithdrawEmptyVault(address,address,uint256,uint256) (runs: 5096, μ: 15316, ~: 15316) -OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516719745, ~: 8937393460516738938) +OrderBookWithdrawTest:testWithdrawFailure(address,uint256,uint256,uint256) (runs: 5096, μ: 8937393460516719607, ~: 8937393460516700429) OrderBookWithdrawTest:testWithdrawFullVault(address,uint256,uint256,uint256) (runs: 5096, μ: 41257, ~: 41256) -OrderBookWithdrawTest:testWithdrawMany((bool,address,address,uint256,uint248)[]) (runs: 5096, μ: 1447473, ~: 1381991) +OrderBookWithdrawTest:testWithdrawMany((bool,address,address,uint256,uint248)[]) (runs: 5096, μ: 1448254, ~: 1371051) OrderBookWithdrawTest:testWithdrawPartialVault(address,uint256,uint256,uint256) (runs: 5096, μ: 51929, ~: 51929) -OrderBookWithdrawTest:testWithdrawReentrant(address,uint256,uint256,address,address,uint256) (runs: 5096, μ: 506060, ~: 507997) +OrderBookWithdrawTest:testWithdrawReentrant(address,uint256,uint256,address,address,uint256) (runs: 5096, μ: 506329, ~: 507997) OrderBookWithdrawTest:testWithdrawZero(address,address,uint256) (runs: 5096, μ: 12787, ~: 12787) \ No newline at end of file diff --git a/src/concrete/OrderBook.sol b/src/concrete/OrderBook.sol index 436721222..41b8c5de5 100644 --- a/src/concrete/OrderBook.sol +++ b/src/concrete/OrderBook.sol @@ -37,6 +37,12 @@ error NotOrderOwner(address sender, address owner); /// @param bobToken The input or output of the other order that doesn't match a. error TokenMismatch(address aliceToken, address bobToken); +/// Thrown when the input and output token decimals don't match, in either +/// direction. +/// @param aliceTokenDecimals The input or output decimals of one order. +/// @param bobTokenDecimals The input or output decimals of the other order. +error TokenDecimalsMismatch(uint8 aliceTokenDecimals, uint8 bobTokenDecimals); + /// Thrown when the minimum input is not met. /// @param minimumInput The minimum input required. /// @param input The input that was achieved. @@ -149,7 +155,9 @@ bytes32 constant CALLER_META_HASH = bytes32(0xf0c79e4006636a71899066ac45a478da4e /// @param kvs KVs returned from calculate order entrypoint to pass to the store /// before calling handle IO entrypoint. struct OrderIOCalculation { - uint256 outputMax; + Order order; + uint256 outputIOIndex; + Output18Amount outputMax; //solhint-disable-next-line var-name-mixedcase uint256 IORatio; uint256[][] context; @@ -157,6 +165,10 @@ struct OrderIOCalculation { uint256[] kvs; } +type Output18Amount is uint256; + +type Input18Amount is uint256; + /// @title OrderBook /// See `IOrderBookV1` for more documentation. contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3FlashLender, DeployerDiscoverableMetaV2 { @@ -325,28 +337,33 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash function takeOrders(TakeOrdersConfigV2 calldata config) external nonReentrant - returns (uint256 totalInput, uint256 totalOutput) + returns (uint256 totalTakerInput, uint256 totalTakerOutput) { uint256 i = 0; - TakeOrderConfig memory takeOrder; + TakeOrderConfig memory takeOrderConfig; Order memory order; - uint256 remainingInput = config.maximumInput; - while (i < config.orders.length && remainingInput > 0) { - takeOrder = config.orders[i]; - order = takeOrder.order; + + uint256 remainingTakerInput = config.maximumInput; + while (i < config.orders.length && remainingTakerInput > 0) { + takeOrderConfig = config.orders[i]; + order = takeOrderConfig.order; bytes32 orderHash = order.hash(); if (sOrders[orderHash] == ORDER_DEAD) { emit OrderNotFound(msg.sender, order.owner, orderHash); } else { - if (order.validInputs[takeOrder.inputIOIndex].token != config.output) { - revert TokenMismatch(order.validInputs[takeOrder.inputIOIndex].token, config.output); + if (order.validInputs[takeOrderConfig.inputIOIndex].token != config.output) { + revert TokenMismatch(order.validInputs[takeOrderConfig.inputIOIndex].token, config.output); } - if (order.validOutputs[takeOrder.outputIOIndex].token != config.input) { - revert TokenMismatch(order.validOutputs[takeOrder.outputIOIndex].token, config.input); + if (order.validOutputs[takeOrderConfig.outputIOIndex].token != config.input) { + revert TokenMismatch(order.validOutputs[takeOrderConfig.outputIOIndex].token, config.input); } OrderIOCalculation memory orderIOCalculation = calculateOrderIO( - order, takeOrder.inputIOIndex, takeOrder.outputIOIndex, msg.sender, takeOrder.signedContext + order, + takeOrderConfig.inputIOIndex, + takeOrderConfig.outputIOIndex, + msg.sender, + takeOrderConfig.signedContext ); // Skip orders that are too expensive rather than revert as we have @@ -355,20 +372,47 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // 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 (orderIOCalculation.outputMax == 0) { + } else if (Output18Amount.unwrap(orderIOCalculation.outputMax) == 0) { emit OrderZeroAmount(msg.sender, order.owner, orderHash); } else { - // Don't exceed the maximum total input. - uint256 input = - remainingInput > orderIOCalculation.outputMax ? orderIOCalculation.outputMax : remainingInput; - // Always round IO calculations up. - uint256 output = input.fixedPointMul(orderIOCalculation.IORatio, Math.Rounding.Up); - - remainingInput -= input; - totalOutput += output; - - recordVaultIO(order, output, input, orderIOCalculation); - emit TakeOrder(msg.sender, takeOrder, input, output); + 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 takerInput = Input18Amount.unwrap(takerInput18).scaleN(takerInputDecimals, FLAG_SATURATE); + + remainingTakerInput -= takerInput; + totalTakerOutput += takerOutput; + + recordVaultIO(order, takerOutput, takerInput, orderIOCalculation); + emit TakeOrder(msg.sender, takeOrderConfig, takerInput, takerOutput); } } @@ -376,10 +420,10 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash i++; } } - totalInput = config.maximumInput - remainingInput; + totalTakerInput = config.maximumInput - remainingTakerInput; - if (totalInput < config.minimumInput) { - revert MinimumInput(config.minimumInput, totalInput); + if (totalTakerInput < config.minimumInput) { + revert MinimumInput(config.minimumInput, totalTakerInput); } // Prioritise paying down any active flash loans before sending any @@ -393,17 +437,19 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash // 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 inputAmountSent = _decreaseFlashDebtThenSendToken(config.input, msg.sender, totalInput); + uint256 inputAmountSent = _decreaseFlashDebtThenSendToken(config.input, msg.sender, totalTakerInput); if (config.data.length > 0) { IOrderBookV3OrderTaker(msg.sender).onTakeOrders( - config.input, config.output, inputAmountSent, totalOutput, config.data + config.input, config.output, inputAmountSent, totalTakerOutput, config.data ); } - // 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.output).safeTransferFrom(msg.sender, address(this), totalOutput); + 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.output).safeTransferFrom(msg.sender, address(this), totalTakerOutput); + } } /// @inheritdoc IOrderBookV3 @@ -428,6 +474,16 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash ); } + if ( + alice.validOutputs[clearConfig.aliceOutputIOIndex].decimals + != bob.validInputs[clearConfig.bobInputIOIndex].decimals + ) { + revert TokenDecimalsMismatch( + alice.validOutputs[clearConfig.aliceOutputIOIndex].decimals, + bob.validInputs[clearConfig.bobInputIOIndex].decimals + ); + } + if ( bob.validOutputs[clearConfig.bobOutputIOIndex].token != alice.validInputs[clearConfig.aliceInputIOIndex].token @@ -438,6 +494,16 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash ); } + if ( + bob.validOutputs[clearConfig.bobOutputIOIndex].decimals + != alice.validInputs[clearConfig.aliceInputIOIndex].decimals + ) { + revert TokenDecimalsMismatch( + alice.validInputs[clearConfig.aliceInputIOIndex].decimals, + bob.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`. @@ -459,28 +525,28 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash OrderIOCalculation memory bobOrderIOCalculation_ = calculateOrderIO( bob, clearConfig.bobInputIOIndex, clearConfig.bobOutputIOIndex, alice.owner, aliceSignedContext ); - ClearStateChange memory clearStateChange_ = + ClearStateChange memory clearStateChange = calculateClearStateChange(aliceOrderIOCalculation_, bobOrderIOCalculation_); - recordVaultIO(alice, clearStateChange_.aliceInput, clearStateChange_.aliceOutput, aliceOrderIOCalculation_); - recordVaultIO(bob, clearStateChange_.bobInput, clearStateChange_.bobOutput, bobOrderIOCalculation_); + recordVaultIO(alice, clearStateChange.aliceInput, clearStateChange.aliceOutput, aliceOrderIOCalculation_); + recordVaultIO(bob, clearStateChange.bobInput, clearStateChange.bobOutput, bobOrderIOCalculation_); { // At least one of these will overflow due to negative bounties if // there is a spread between the orders. - uint256 aliceBounty_ = clearStateChange_.aliceOutput - clearStateChange_.bobInput; - uint256 bobBounty_ = clearStateChange_.bobOutput - clearStateChange_.aliceInput; - if (aliceBounty_ > 0) { + uint256 aliceBounty = clearStateChange.aliceOutput - clearStateChange.bobInput; + uint256 bobBounty = clearStateChange.bobOutput - clearStateChange.aliceInput; + if (aliceBounty > 0) { sVaultBalances[msg.sender][alice.validOutputs[clearConfig.aliceOutputIOIndex].token][clearConfig - .aliceBountyVaultId] += aliceBounty_; + .aliceBountyVaultId] += aliceBounty; } - if (bobBounty_ > 0) { + if (bobBounty > 0) { sVaultBalances[msg.sender][bob.validOutputs[clearConfig.bobOutputIOIndex].token][clearConfig - .bobBountyVaultId] += bobBounty_; + .bobBountyVaultId] += bobBounty; } } - emit AfterClear(msg.sender, clearStateChange_); + emit AfterClear(msg.sender, clearStateChange); } /// Main entrypoint into an order calculates the amount and IO ratio. Both @@ -548,45 +614,41 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash .interpreter .eval(order.evaluable.store, namespace, _calculateOrderDispatch(order.evaluable.expression), context); - uint256 orderOutputMax = calculateOrderStack[calculateOrderStack.length - 2]; + Output18Amount orderOutputMax18 = Output18Amount.wrap(calculateOrderStack[calculateOrderStack.length - 2]); uint256 orderIORatio = calculateOrderStack[calculateOrderStack.length - 1]; - // Rescale order output max from 18 FP to whatever decimals the - // output token is using. - // Always round order output down. - orderOutputMax = orderOutputMax.scaleN( - order.validOutputs[outputIOIndex].decimals, - // Saturate the order max output because if we were willing to - // give more than this on a scale up, we should be comfortable - // giving less. - // Round DOWN to be conservative and give away less if there's - // any loss of precision during scale down. - FLAG_SATURATE - ); - // Rescale the ratio from 18 FP according to the difference in - // decimals between input and output. - // Always round IO ratio up. - orderIORatio = orderIORatio.scaleRatio( - order.validOutputs[outputIOIndex].decimals, - order.validInputs[inputIOIndex].decimals, - // DO NOT saturate ratios because this would reduce the effective - // IO ratio, which would mean that saturating would make the deal - // worse for the order. Instead we overflow, and round up to get - // the best possible deal. - FLAG_ROUND_UP - ); - - // The order owner can't send more than the smaller of their vault - // balance or their per-order limit. - uint256 ownerVaultBalance = sVaultBalances[order.owner][order.validOutputs[outputIOIndex].token][order - .validOutputs[outputIOIndex].vaultId]; - orderOutputMax = orderOutputMax > ownerVaultBalance ? ownerVaultBalance : orderOutputMax; + { + // The order owner can't send more than the smaller of their vault + // balance or their per-order limit. + uint256 ownerVaultBalance = sVaultBalances[order.owner][order.validOutputs[outputIOIndex].token][order + .validOutputs[outputIOIndex].vaultId]; + // We round down vault balances and don't saturate because we're + // dealing with real token amounts here. If rescaling would somehow + // cause an overflow in a real token amount, that's basically an + // unsupported token, it implies a very small decimals value with + // very large token total supply. E.g. 0 decimals with a total supply + // around 10^60. That's beyond what even Uniswap handles, as they use + // uint112 values internally for tokens. + // It's possible that if a token has large decimals, e.g. much more + // than 18, that the owner vault balance could be rounded down enough + // to cause significant non-dust amounts to be untradeable. In this + // case the token is not really supported. + // In either case, the order owner can still withdraw their vault + // balances in full, they just can't trade that token effectively. + Output18Amount ownerVaultBalance18 = + Output18Amount.wrap(ownerVaultBalance.scale18(order.validOutputs[outputIOIndex].decimals, 0)); + if (Output18Amount.unwrap(orderOutputMax18) > Output18Amount.unwrap(ownerVaultBalance18)) { + orderOutputMax18 = ownerVaultBalance18; + } + } - // Populate the context with the output max rescaled and vault capped - // and the rescaled ratio. - context[CONTEXT_CALCULATIONS_COLUMN] = LibUint256Array.arrayFrom(orderOutputMax, orderIORatio); + // Populate the context with the output max rescaled and vault capped. + context[CONTEXT_CALCULATIONS_COLUMN] = + LibUint256Array.arrayFrom(Output18Amount.unwrap(orderOutputMax18), orderIORatio); - return OrderIOCalculation(orderOutputMax, orderIORatio, context, namespace, calculateOrderKVs); + return OrderIOCalculation( + order, outputIOIndex, orderOutputMax18, orderIORatio, context, namespace, calculateOrderKVs + ); } } @@ -681,30 +743,48 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash OrderIOCalculation memory aliceOrderIOCalculation, OrderIOCalculation memory bobOrderIOCalculation ) internal pure returns (ClearStateChange memory clearStateChange) { - // Alice's output is the smaller of their max output and Bob's input. - clearStateChange.aliceOutput = aliceOrderIOCalculation.outputMax.min( - // Bob's input is Alice's output. - // Alice cannot output more than their max. - // Bob wants input of their IO ratio * their output. - // Always round IO calculations up. - bobOrderIOCalculation.outputMax.fixedPointMul(bobOrderIOCalculation.IORatio, Math.Rounding.Up) + calculateClearStateAlice(clearStateChange, aliceOrderIOCalculation, bobOrderIOCalculation); + // Flip alice and bob to calculate bob's output. + calculateClearStateAlice(clearStateChange, bobOrderIOCalculation, aliceOrderIOCalculation); + } + + function calculateClearStateAlice( + ClearStateChange memory clearStateChange, + OrderIOCalculation memory aliceOrderIOCalculation, + OrderIOCalculation memory bobOrderIOCalculation + ) internal pure { + // Always round IO calculations up so that the counterparty pays more. + // This is the max input that bob can afford, given his own IO ratio + // and maximum spend/output. + Input18Amount bobInputMax18 = Input18Amount.wrap( + Output18Amount.unwrap(bobOrderIOCalculation.outputMax).fixedPointMul( + bobOrderIOCalculation.IORatio, Math.Rounding.Up + ) ); - // Bob's output is the smaller of their max output and Alice's input. - clearStateChange.bobOutput = bobOrderIOCalculation.outputMax.min( - // Alice's input is Bob's output. - // Bob cannot output more than their max. - // Alice wants input of their IO ratio * their output. - // Always round IO calculations up. - aliceOrderIOCalculation.outputMax.fixedPointMul(aliceOrderIOCalculation.IORatio, Math.Rounding.Up) + Output18Amount aliceOutputMax18 = aliceOrderIOCalculation.outputMax; + // Alice's doesn't need to provide more output than bob's max input. + if (Output18Amount.unwrap(aliceOutputMax18) > Input18Amount.unwrap(bobInputMax18)) { + aliceOutputMax18 = Output18Amount.wrap(Input18Amount.unwrap(bobInputMax18)); + } + // Alice's final output is the scaled version of the 18 decimal output, + // rounded down to benefit Alice. + clearStateChange.aliceOutput = Output18Amount.unwrap(aliceOutputMax18).scaleN( + aliceOrderIOCalculation.order.validOutputs[aliceOrderIOCalculation.outputIOIndex].decimals, 0 + ); + + // Alice's input is her bob-capped output * her IO ratio, rounded up. + Input18Amount aliceInput18 = Input18Amount.wrap( + Output18Amount.unwrap(aliceOutputMax18).fixedPointMul(aliceOrderIOCalculation.IORatio, Math.Rounding.Up) ); - // Alice's input is Alice's output * their IO ratio. - // Always round IO calculations up. clearStateChange.aliceInput = - clearStateChange.aliceOutput.fixedPointMul(aliceOrderIOCalculation.IORatio, Math.Rounding.Up); - // Bob's input is Bob's output * their IO ratio. - // Always round IO calculations up. - clearStateChange.bobInput = - clearStateChange.bobOutput.fixedPointMul(bobOrderIOCalculation.IORatio, Math.Rounding.Up); + // Use bob's output decimals as alice's input decimals. + // + // This is only safe if we have previously checked that the decimals + // match for alice and bob per token, otherwise bob could manipulate + // alice's intent. + Input18Amount.unwrap(aliceInput18).scaleN( + bobOrderIOCalculation.order.validOutputs[bobOrderIOCalculation.outputIOIndex].decimals, FLAG_ROUND_UP + ); } function _calculateOrderDispatch(address expression_) internal pure returns (EncodedDispatch) { diff --git a/test/concrete/OrderBook.takeOrder.t.sol b/test/concrete/OrderBook.takeOrder.t.sol new file mode 100644 index 000000000..ff9213176 --- /dev/null +++ b/test/concrete/OrderBook.takeOrder.t.sol @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: CAL +pragma solidity =0.8.19; + +import "test/util/abstract/OrderBookExternalRealTest.sol"; + +/// @title OrderBookTakeOrderTest +/// @notice A test harness for testing the OrderBook takeOrder function. +contract OrderBookTakeOrderTest is OrderBookExternalRealTest { + function checkPrecision( + bytes memory rainString, + uint8 outputTokenDecimals, + uint8 inputTokenDecimals, + uint256 expectedTakerTotalInput, + uint256 expectedTakerTotalOutput + ) internal { + uint256 vaultId = 0; + address inputToken = address(0x100); + address outputToken = address(0x101); + OrderConfigV2 memory config; + { + IO[] memory validInputs = new IO[](1); + validInputs[0] = IO(inputToken, inputTokenDecimals, vaultId); + IO[] memory validOutputs = new IO[](1); + validOutputs[0] = IO(outputToken, outputTokenDecimals, vaultId); + // These numbers are known to cause large rounding errors if the + // precision is not handled correctly. + (bytes memory bytecode, uint256[] memory constants) = IParserV1(address(iDeployer)).parse(rainString); + 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)); + } + if (expectedTakerTotalInput > 0) { + iOrderbook.deposit(outputToken, vaultId, expectedTakerTotalInput); + } + assertEq(iOrderbook.vaultBalance(address(this), outputToken, vaultId), expectedTakerTotalInput); + 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(inputToken, outputToken, 0, type(uint256).max, type(uint256).max, orders, ""); + (uint256 totalTakerInput, uint256 totalTakerOutput) = iOrderbook.takeOrders(takeOrdersConfig); + assertEq(totalTakerInput, expectedTakerTotalInput); + assertEq(totalTakerOutput, expectedTakerTotalOutput); + assertEq(iOrderbook.vaultBalance(address(this), outputToken, vaultId), 0); + } + + function testTakeOrderPrecisionKnownBad01() public { + // Older versions of OB had precision issues with this IO setup. + bytes memory knownBad = "output-max io-ratio:157116365680491867129910 318235466963885;:;"; + // Start with both tokens having 18 decimals. + // This gives the best precision for both. + checkPrecision(knownBad, 18, 18, 157116365680491867129910, 49999999999999844580); + + // If the taker output token has low decimals then it will round up + // at that decimal precision, to force the taker to have to output the + // dust amount. + // Increasing the decimals of the taker input token will not impact the + // precision provided there is no overflow. It simply scales up the + // taker input amount. + checkPrecision(knownBad, 18, 6, 157116365680491867129910, 50e6); + checkPrecision(knownBad, 19, 6, 1571163656804918671299100, 50e6); + checkPrecision(knownBad, 20, 6, 15711636568049186712991000, 50e6); + checkPrecision(knownBad, 21, 6, 157116365680491867129910000, 50e6); + checkPrecision(knownBad, 50, 6, 15711636568049186712991000000000000000000000000000000000, 50e6); + + // Flip the decimals for each token. + // As the output has low decimals, it rounds down which then causes the + // input to be slightly smaller prorata. + checkPrecision(knownBad, 6, 18, 157116365680, 49999999999843315014); + checkPrecision(knownBad, 6, 19, 157116365680, 499999999998433150140); + checkPrecision(knownBad, 6, 20, 157116365680, 4999999999984331501400); + checkPrecision(knownBad, 6, 21, 157116365680, 49999999999843315014000); + checkPrecision(knownBad, 6, 50, 157116365680, 4999999999984331501400000000000000000000000000000000); + } +}