Reject foreign onTakeOrders2 callers during arb5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: thedavidmeister

src/abstract/RaindexV6ArbOrderTaker.sol

@@ -19,6 +19,10 @@ import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTO
 /// and calls it, so an untrusted address could drain the contract's balance.
 error BadRaindex(address badRaindex);
 
+/// Thrown when `onTakeOrders2` is called during an arb by anything other than
+/// the trusted raindex.
+error UnexpectedOnTakeOrdersCaller(address caller);
+
 /// @title RaindexV6ArbOrderTaker
 /// @notice Arb contract that takes orders directly from a `Raindex` without
 /// flash loans. Inheritors implement the strategy for sourcing the input token
@@ -75,19 +79,24 @@ abstract contract RaindexV6ArbOrderTaker is
     }
 
     /// @inheritdoc IRaindexV6OrderTaker
-    /// @dev Empty no-op, and intentionally permissionless: the contract holds no
-    /// value between operations, so there is nothing to protect via `msg.sender`
-    /// validation.
+    /// @dev While `arb5` is in flight only the trusted raindex may call this;
+    /// any other caller reverts with `UnexpectedOnTakeOrdersCaller`. During an
+    /// arb the contract holds the caller's `msg.value` and tokens, and
+    /// `takeOrders4` runs order-owner-chosen code (e.g. the order's store) that
+    /// could otherwise re-enter here and direct that value.
     ///
-    /// The permissionless entry is also the recovery path for value that reaches
-    /// the contract by accident. Unsolicited transfers cannot be prevented: ERC20
-    /// transfers have no recipient hook to revert in, and the arb legitimately
-    /// receives ERC20 and ETH mid-operation. Because anyone may call the
-    /// take-orders entrypoints, a stray balance is swept out by the next call
-    /// rather than stranded; gating this hook to an active orderbook callback
-    /// would instead brick such balances permanently. Concrete implementations
-    /// that move value here (e.g. the generic-pool variant's unlimited
-    /// approve-and-call with whole-ETH-balance forward) are safe only under this
-    /// no-value-between-operations invariant.
-    function onTakeOrders2(address, address, Float, Float, bytes calldata) public virtual override {}
+    /// Outside an arb it is a permissionless no-op: the contract holds no value
+    /// between operations, so there is nothing to protect. The permissionless
+    /// entry is the recovery path for value that reaches the contract by
+    /// accident. Unsolicited transfers cannot be prevented: ERC20 transfers have
+    /// no recipient hook to revert in, and the arb legitimately receives ERC20
+    /// and ETH mid-operation. A stray balance is swept out by the next call
+    /// rather than stranded. Concrete implementations that move value here (e.g.
+    /// the generic-pool variant's unlimited approve-and-call with whole-ETH-
+    /// balance forward) rely on both properties.
+    function onTakeOrders2(address, address, Float, Float, bytes calldata) public virtual override {
+        if (_reentrancyGuardEntered() && msg.sender != LibRaindexDeploy.RAINDEX_DEPLOYED_ADDRESS) {
+            revert UnexpectedOnTakeOrdersCaller(msg.sender);
+        }
+    }
 }

src/generated/candidate/GenericPoolRaindexV6ArbOrderTaker.sol

@@ -11,12 +11,12 @@ import {LibRainDeploy} from "rain-deploy-0.1.22/src/lib/LibRainDeploy.sol";
 import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTOFUTokenDecimals.sol";
 import {MockToken} from "test/util/concrete/MockToken.sol";
 
-/// The abstract `RaindexV6ArbOrderTaker.onTakeOrders2` body is a pure no-op:
+/// Outside an arb the abstract `RaindexV6ArbOrderTaker.onTakeOrders2` body is a no-op:
 /// it neither reverts nor touches state, and it does NOT decode or act on its
 /// `bytes` argument. A non-overriding child (`ChildRaindexV6ArbOrderTaker`)
 /// exercises the base directly.
 contract RaindexV6ArbOrderTakerOnTakeOrders2BaseTest is Test {
-    /// The base no-op does nothing: it succeeds for ANY caller and ANY
+    /// Outside an arb the base no-op does nothing: it succeeds for ANY caller and ANY
     /// arguments (including non-decodable garbage `bytes`), and leaves token
     /// balances untouched. Inserting a `revert` into the base body, or making
     /// it decode/act on the data, breaks this.

src/generated/candidate/RouteProcessorRaindexV6ArbOrderTaker.sol

@@ -11,8 +11,8 @@ import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTO
 import {MockToken} from "test/util/concrete/MockToken.sol";
 
 contract RaindexV6ArbOrderTakerOnTakeOrders2DirectTest is Test {
-    /// Calling onTakeOrders2 directly MUST succeed (no-op) and not modify
-    /// any state. Anyone can call it — there is no access control by design.
+    /// Calling onTakeOrders2 directly outside an arb MUST succeed (no-op) and not modify
+    /// any state, whoever the caller.
     function testOnTakeOrders2DirectCallSucceeds() external {
         LibRainDeploy.etchZoltuFactory(vm);
         LibRainDeploy.deployZoltu(LibTOFUTokenDecimals.TOFU_DECIMALS_EXPECTED_CREATION_CODE);

test/abstract/RaindexV6ArbOrderTaker.onTakeOrders2Base.t.sol

@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: LicenseRef-DCL-1.0
+// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
+pragma solidity =0.8.25;
+
+import {RaindexV6ExternalRealTest} from "test/util/abstract/RaindexV6ExternalRealTest.sol";
+import {MockToken} from "test/util/concrete/MockToken.sol";
+import {LibTestTakeOrder} from "test/util/lib/LibTestTakeOrder.sol";
+import {LibTestArb} from "test/util/lib/LibTestArb.sol";
+import {GenericPoolRaindexV6ArbOrderTaker} from "../../../src/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.sol";
+import {UnexpectedOnTakeOrdersCaller} from "../../../src/abstract/RaindexV6ArbOrderTaker.sol";
+import {
+    OrderV4,
+    OrderConfigV4,
+    IOV2,
+    EvaluableV4,
+    TaskV2,
+    TakeOrdersConfigV5
+} from "raindex-interface-0.1.8/src/interface/IRaindexV6.sol";
+import {IInterpreterV4, StateNamespace} from "rainlang-interface-0.2.13/src/interface/IInterpreterV4.sol";
+import {IInterpreterStoreV3} from "rainlang-interface-0.2.13/src/interface/IInterpreterStoreV3.sol";
+import {TEST_INTERPRETER_ADDRESS} from "rainlang-0.2.19/test/lib/deploy/LibTestInterpreterDeploy.sol";
+import {Float, LibDecimalFloat} from "rain-math-float-0.3.10/src/lib/LibDecimalFloat.sol";
+
+/// Store that, on `set`, calls the arb's `onTakeOrders2` naming itself as
+/// spender and pool, and records the revert data.
+contract ReentrantOnTakeOrdersStore {
+    address payable public arb;
+    address public token;
+    bytes public revertData;
+
+    function arm(address payable arb_, address token_) external {
+        arb = arb_;
+        token = token_;
+    }
+
+    function set(StateNamespace, bytes32[] calldata) external {
+        try GenericPoolRaindexV6ArbOrderTaker(arb)
+            .onTakeOrders2(
+                token, token, Float.wrap(0), Float.wrap(0), abi.encode(address(this), address(this), bytes(""))
+            ) {}
+        catch (bytes memory err) {
+            revertData = err;
+        }
+    }
+
+    function get(bytes32, bytes32) external pure returns (bytes32) {
+        return 0;
+    }
+
+    fallback() external payable {}
+    receive() external payable {}
+}
+
+/// Takes `amount` tokenIn from the caller, mints `amount` tokenOut to it and
+/// refunds any ETH sent.
+contract OneToOnePool {
+    function swap(MockToken tokenIn, MockToken tokenOut, uint256 amount) external payable {
+        tokenIn.transferFrom(msg.sender, address(this), amount);
+        tokenOut.mint(msg.sender, amount);
+        if (msg.value > 0) payable(msg.sender).transfer(msg.value);
+    }
+}
+
+contract EthSink {
+    receive() external payable {}
+    fallback() external payable {}
+}
+
+contract GenericPoolRaindexV6ArbOrderTakerReentrantCallbackTest is RaindexV6ExternalRealTest {
+    address internal owner = address(uint160(uint256(keccak256("owner.reentrant.callback"))));
+    address internal bot = address(uint160(uint256(keccak256("bot.reentrant.callback"))));
+
+    /// An order's store re-entering `onTakeOrders2` during `arb5` is rejected,
+    /// the arb completes and the caller's ETH is refunded to the caller.
+    function testOrderStoreReentrantCallbackCannotTakeArbEth() external {
+        GenericPoolRaindexV6ArbOrderTaker arb = new GenericPoolRaindexV6ArbOrderTaker();
+        MockToken token0 = new MockToken("T0", "T0", 18);
+        MockToken token1 = new MockToken("T1", "T1", 18);
+        ReentrantOnTakeOrdersStore store = new ReentrantOnTakeOrdersStore();
+        store.arm(payable(address(arb)), address(token0));
+        OneToOnePool pool = new OneToOnePool();
+
+        token1.mint(owner, 10e18);
+        vm.startPrank(owner);
+        token1.approve(address(iRaindex), 10e18);
+        iRaindex.deposit4(address(token1), bytes32(uint256(1)), LibDecimalFloat.packLossless(10, 0), new TaskV2[](0));
+        vm.stopPrank();
+
+        IOV2[] memory ins = new IOV2[](1);
+        ins[0] = IOV2(address(token0), bytes32(uint256(1)));
+        IOV2[] memory outs = new IOV2[](1);
+        outs[0] = IOV2(address(token1), bytes32(uint256(1)));
+        OrderConfigV4 memory cfg = OrderConfigV4(
+            EvaluableV4(
+                IInterpreterV4(TEST_INTERPRETER_ADDRESS),
+                IInterpreterStoreV3(address(store)),
+                iParserV2.parse2(":set(1 2),\n_ _: 1 1;:;")
+            ),
+            ins,
+            outs,
+            bytes32(0),
+            bytes32(0),
+            ""
+        );
+        vm.recordLogs();
+        vm.prank(owner);
+        iRaindex.addOrder4(cfg, new TaskV2[](0));
+        OrderV4 memory order = LibTestTakeOrder.extractOrderFromLogs(vm.getRecordedLogs());
+
+        TakeOrdersConfigV5 memory take = LibTestTakeOrder.defaultTakeConfig(LibTestTakeOrder.wrapSingle(order));
+        take.maximumIO = LibDecimalFloat.packLossless(1, 0);
+        take.data = abi.encode(address(pool), address(pool), abi.encodeCall(OneToOnePool.swap, (token1, token0, 1e18)));
+
+        vm.deal(bot, 1 ether);
+        vm.prank(bot);
+        arb.arb5{value: 1 ether}(iRaindex, take, LibTestArb.noopTask());
+
+        assertEq(store.revertData(), abi.encodeWithSelector(UnexpectedOnTakeOrdersCaller.selector, address(store)));
+        assertEq(address(store).balance, 0);
+        assertEq(bot.balance, 1 ether);
+        assertEq(token1.balanceOf(address(pool)), 1e18);
+        assertEq(token0.balanceOf(owner), 0);
+        assertEq(address(arb).balance, 0);
+    }
+
+    /// Outside an arb, anyone may call `onTakeOrders2` and it forwards a stray
+    /// ETH balance to the named pool.
+    function testOnTakeOrders2OutsideArbSweepsStrayEth(address caller) external {
+        GenericPoolRaindexV6ArbOrderTaker arb = new GenericPoolRaindexV6ArbOrderTaker();
+        MockToken token = new MockToken("T", "T", 18);
+        EthSink sink = new EthSink();
+        vm.deal(address(arb), 1 ether);
+
+        vm.prank(caller);
+        arb.onTakeOrders2(
+            address(token), address(token), Float.wrap(0), Float.wrap(0), abi.encode(address(sink), address(sink), "")
+        );
+
+        assertEq(address(sink).balance, 1 ether);
+        assertEq(address(arb).balance, 0);
+    }
+}

test/abstract/RaindexV6ArbOrderTaker.onTakeOrders2Direct.t.sol

@@ -14,7 +14,7 @@ import {MockRouteProcessor} from "test/util/concrete/MockRouteProcessor.sol";
 
 contract RouteProcessorRaindexV6ArbOrderTakerOnTakeOrders2DirectTest is Test {
     /// Calling onTakeOrders2 directly from an arbitrary address MUST succeed.
-    /// The function has no access control by design — the contract is stateless
+    /// Outside an arb the function has no access control — the contract is stateless
     /// between operations so there is nothing to exploit.
     function testOnTakeOrders2DirectCallByAttacker() external {
         LibRainDeploy.etchZoltuFactory(vm);