wip on reentrant calculate orders

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"
 }

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.

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";

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.

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,38 @@ 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();
+        // 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 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 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`. 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);
 
         return true;
     }
@@ -195,6 +66,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));
     }
 }