Skip to content

Commit 3afdf51

Browse files
thedavidmeisterthedavidmeister
authored
Merge pull request #2914 from rainlanguage/fix/2907-generic-pool-reentrant-callback
Reject foreign onTakeOrders2 callers during arb5
2 parents 785b652 + 9a8c6ba commit 3afdf51

8 files changed

Lines changed: 176 additions & 27 deletions

‎src/abstract/RaindexV6ArbOrderTaker.sol‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTO
1919
/// and calls it, so an untrusted address could drain the contract's balance.
2020
error BadRaindex(address badRaindex);
2121

22+
/// Thrown when `onTakeOrders2` is called during an arb by anything other than
23+
/// the trusted raindex.
24+
error UnexpectedOnTakeOrdersCaller(address caller);
25+
2226
/// @title RaindexV6ArbOrderTaker
2327
/// @notice Arb contract that takes orders directly from a `Raindex` without
2428
/// flash loans. Inheritors implement the strategy for sourcing the input token
@@ -75,19 +79,24 @@ abstract contract RaindexV6ArbOrderTaker is
7579
}
7680

7781
/// @inheritdoc IRaindexV6OrderTaker
78-
/// @dev Empty no-op, and intentionally permissionless: the contract holds no
79-
/// value between operations, so there is nothing to protect via `msg.sender`
80-
/// validation.
82+
/// @dev While `arb5` is in flight only the trusted raindex may call this;
83+
/// any other caller reverts with `UnexpectedOnTakeOrdersCaller`. During an
84+
/// arb the contract holds the caller's `msg.value` and tokens, and
85+
/// `takeOrders4` runs order-owner-chosen code (e.g. the order's store) that
86+
/// could otherwise re-enter here and direct that value.
8187
///
82-
/// The permissionless entry is also the recovery path for value that reaches
83-
/// the contract by accident. Unsolicited transfers cannot be prevented: ERC20
84-
/// transfers have no recipient hook to revert in, and the arb legitimately
85-
/// receives ERC20 and ETH mid-operation. Because anyone may call the
86-
/// take-orders entrypoints, a stray balance is swept out by the next call
87-
/// rather than stranded; gating this hook to an active orderbook callback
88-
/// would instead brick such balances permanently. Concrete implementations
89-
/// that move value here (e.g. the generic-pool variant's unlimited
90-
/// approve-and-call with whole-ETH-balance forward) are safe only under this
91-
/// no-value-between-operations invariant.
92-
function onTakeOrders2(address, address, Float, Float, bytes calldata) public virtual override {}
88+
/// Outside an arb it is a permissionless no-op: the contract holds no value
89+
/// between operations, so there is nothing to protect. The permissionless
90+
/// entry is the recovery path for value that reaches the contract by
91+
/// accident. Unsolicited transfers cannot be prevented: ERC20 transfers have
92+
/// no recipient hook to revert in, and the arb legitimately receives ERC20
93+
/// and ETH mid-operation. A stray balance is swept out by the next call
94+
/// rather than stranded. Concrete implementations that move value here (e.g.
95+
/// the generic-pool variant's unlimited approve-and-call with whole-ETH-
96+
/// balance forward) rely on both properties.
97+
function onTakeOrders2(address, address, Float, Float, bytes calldata) public virtual override {
98+
if (_reentrancyGuardEntered() && msg.sender != LibRaindexDeploy.RAINDEX_DEPLOYED_ADDRESS) {
99+
revert UnexpectedOnTakeOrdersCaller(msg.sender);
100+
}
101+
}
93102
}

‎src/generated/candidate/GenericPoolRaindexV6ArbOrderTaker.sol‎

Lines changed: 4 additions & 4 deletions
Large diffs are not rendered by default.

‎src/generated/candidate/RouteProcessorRaindexV6ArbOrderTaker.sol‎

Lines changed: 4 additions & 4 deletions
Large diffs are not rendered by default.

‎test/abstract/RaindexV6ArbOrderTaker.onTakeOrders2Base.t.sol‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,12 @@ import {LibRainDeploy} from "rain-deploy-0.1.22/src/lib/LibRainDeploy.sol";
1111
import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTOFUTokenDecimals.sol";
1212
import {MockToken} from "test/util/concrete/MockToken.sol";
1313

14-
/// The abstract `RaindexV6ArbOrderTaker.onTakeOrders2` body is a pure no-op:
14+
/// Outside an arb the abstract `RaindexV6ArbOrderTaker.onTakeOrders2` body is a no-op:
1515
/// it neither reverts nor touches state, and it does NOT decode or act on its
1616
/// `bytes` argument. A non-overriding child (`ChildRaindexV6ArbOrderTaker`)
1717
/// exercises the base directly.
1818
contract RaindexV6ArbOrderTakerOnTakeOrders2BaseTest is Test {
19-
/// The base no-op does nothing: it succeeds for ANY caller and ANY
19+
/// Outside an arb the base no-op does nothing: it succeeds for ANY caller and ANY
2020
/// arguments (including non-decodable garbage `bytes`), and leaves token
2121
/// balances untouched. Inserting a `revert` into the base body, or making
2222
/// it decode/act on the data, breaks this.

‎test/abstract/RaindexV6ArbOrderTaker.onTakeOrders2Direct.t.sol‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ import {LibTOFUTokenDecimals} from "rain-tofu-erc20-decimals-0.1.4/src/lib/LibTO
1111
import {MockToken} from "test/util/concrete/MockToken.sol";
1212

1313
contract RaindexV6ArbOrderTakerOnTakeOrders2DirectTest is Test {
14-
/// Calling onTakeOrders2 directly MUST succeed (no-op) and not modify
15-
/// any state. Anyone can call it — there is no access control by design.
14+
/// Calling onTakeOrders2 directly outside an arb MUST succeed (no-op) and not modify
15+
/// any state, whoever the caller.
1616
function testOnTakeOrders2DirectCallSucceeds() external {
1717
LibRainDeploy.etchZoltuFactory(vm);
1818
LibRainDeploy.deployZoltu(LibTOFUTokenDecimals.TOFU_DECIMALS_EXPECTED_CREATION_CODE);

‎test/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.reentrantCallback.t.sol‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
// SPDX-License-Identifier: LicenseRef-DCL-1.0
2+
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
3+
pragma solidity =0.8.25;
4+
5+
import {RaindexV6ExternalRealTest} from "test/util/abstract/RaindexV6ExternalRealTest.sol";
6+
import {MockToken} from "test/util/concrete/MockToken.sol";
7+
import {MockExchange} from "test/util/concrete/MockExchange.sol";
8+
import {FallbackPool} from "test/util/concrete/FallbackPool.sol";
9+
import {ReentrantOnTakeOrdersStore} from "test/util/concrete/ReentrantOnTakeOrdersStore.sol";
10+
import {LibTestTakeOrder} from "test/util/lib/LibTestTakeOrder.sol";
11+
import {LibTestArb} from "test/util/lib/LibTestArb.sol";
12+
import {GenericPoolRaindexV6ArbOrderTaker} from "../../../src/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.sol";
13+
import {UnexpectedOnTakeOrdersCaller} from "../../../src/abstract/RaindexV6ArbOrderTaker.sol";
14+
import {
15+
OrderV4,
16+
OrderConfigV4,
17+
IOV2,
18+
EvaluableV4,
19+
TaskV2,
20+
TakeOrdersConfigV5
21+
} from "raindex-interface-0.1.8/src/interface/IRaindexV6.sol";
22+
import {IInterpreterV4} from "rainlang-interface-0.2.13/src/interface/IInterpreterV4.sol";
23+
import {IInterpreterStoreV3} from "rainlang-interface-0.2.13/src/interface/IInterpreterStoreV3.sol";
24+
import {TEST_INTERPRETER_ADDRESS} from "rainlang-0.2.19/test/lib/deploy/LibTestInterpreterDeploy.sol";
25+
import {Float, LibDecimalFloat} from "rain-math-float-0.3.10/src/lib/LibDecimalFloat.sol";
26+
27+
contract GenericPoolRaindexV6ArbOrderTakerReentrantCallbackTest is RaindexV6ExternalRealTest {
28+
address internal owner = address(uint160(uint256(keccak256("owner.reentrant.callback"))));
29+
address internal bot = address(uint160(uint256(keccak256("bot.reentrant.callback"))));
30+
31+
/// An order's store re-entering `onTakeOrders2` during `arb5` is rejected,
32+
/// the arb completes and the caller's ETH is refunded to the caller.
33+
function testOrderStoreReentrantCallbackCannotTakeArbEth() external {
34+
GenericPoolRaindexV6ArbOrderTaker arb = new GenericPoolRaindexV6ArbOrderTaker();
35+
MockToken token0 = new MockToken("T0", "T0", 18);
36+
MockToken token1 = new MockToken("T1", "T1", 18);
37+
ReentrantOnTakeOrdersStore store = new ReentrantOnTakeOrdersStore(arb, token0);
38+
MockExchange pool = new MockExchange();
39+
token0.mint(address(pool), 1e18);
40+
41+
token1.mint(owner, 10e18);
42+
vm.startPrank(owner);
43+
token1.approve(address(iRaindex), 10e18);
44+
iRaindex.deposit4(address(token1), bytes32(uint256(1)), LibDecimalFloat.packLossless(10, 0), new TaskV2[](0));
45+
vm.stopPrank();
46+
47+
IOV2[] memory ins = new IOV2[](1);
48+
ins[0] = IOV2(address(token0), bytes32(uint256(1)));
49+
IOV2[] memory outs = new IOV2[](1);
50+
outs[0] = IOV2(address(token1), bytes32(uint256(1)));
51+
OrderConfigV4 memory cfg = OrderConfigV4(
52+
EvaluableV4(
53+
IInterpreterV4(TEST_INTERPRETER_ADDRESS),
54+
IInterpreterStoreV3(address(store)),
55+
iParserV2.parse2(":set(1 2),\n_ _: 1 1;:;")
56+
),
57+
ins,
58+
outs,
59+
bytes32(0),
60+
bytes32(0),
61+
""
62+
);
63+
vm.recordLogs();
64+
vm.prank(owner);
65+
iRaindex.addOrder4(cfg, new TaskV2[](0));
66+
OrderV4 memory order = LibTestTakeOrder.extractOrderFromLogs(vm.getRecordedLogs());
67+
68+
TakeOrdersConfigV5 memory take = LibTestTakeOrder.defaultTakeConfig(LibTestTakeOrder.wrapSingle(order));
69+
take.maximumIO = LibDecimalFloat.packLossless(1, 0);
70+
take.data = abi.encode(address(pool), address(pool), abi.encodeCall(MockExchange.swap, (token1, token0, 1e18)));
71+
72+
vm.deal(bot, 1 ether);
73+
vm.prank(bot);
74+
arb.arb5{value: 1 ether}(iRaindex, take, LibTestArb.noopTask());
75+
76+
assertEq(store.revertData(), abi.encodeWithSelector(UnexpectedOnTakeOrdersCaller.selector, address(store)));
77+
assertEq(address(store).balance, 0);
78+
assertEq(bot.balance, 1 ether);
79+
assertEq(token1.balanceOf(address(pool)), 1e18);
80+
assertEq(address(arb).balance, 0);
81+
}
82+
83+
/// Outside an arb, anyone may call `onTakeOrders2` and it forwards a stray
84+
/// ETH balance to the named pool.
85+
function testOnTakeOrders2OutsideArbSweepsStrayEth(address caller) external {
86+
GenericPoolRaindexV6ArbOrderTaker arb = new GenericPoolRaindexV6ArbOrderTaker();
87+
MockToken token = new MockToken("T", "T", 18);
88+
FallbackPool sink = new FallbackPool();
89+
vm.deal(address(arb), 1 ether);
90+
91+
vm.prank(caller);
92+
arb.onTakeOrders2(
93+
address(token), address(token), Float.wrap(0), Float.wrap(0), abi.encode(address(sink), address(sink), "")
94+
);
95+
96+
assertEq(address(sink).balance, 1 ether);
97+
assertEq(address(arb).balance, 0);
98+
}
99+
}

‎test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Direct.t.sol‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {MockRouteProcessor} from "test/util/concrete/MockRouteProcessor.sol";
1414

1515
contract RouteProcessorRaindexV6ArbOrderTakerOnTakeOrders2DirectTest is Test {
1616
/// Calling onTakeOrders2 directly from an arbitrary address MUST succeed.
17-
/// The function has no access control by design — the contract is stateless
17+
/// Outside an arb the function has no access control — the contract is stateless
1818
/// between operations so there is nothing to exploit.
1919
function testOnTakeOrders2DirectCallByAttacker() external {
2020
LibRainDeploy.etchZoltuFactory(vm);

‎test/util/concrete/ReentrantOnTakeOrdersStore.sol‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// SPDX-License-Identifier: LicenseRef-DCL-1.0
2+
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
3+
pragma solidity =0.8.25;
4+
5+
import {GenericPoolRaindexV6ArbOrderTaker} from "../../../src/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.sol";
6+
import {StateNamespace} from "rainlang-interface-0.2.13/src/interface/IInterpreterV4.sol";
7+
import {MockToken} from "./MockToken.sol";
8+
import {Float} from "rain-math-float-0.3.10/src/lib/LibDecimalFloat.sol";
9+
10+
/// Store that, on `set`, calls the arb's `onTakeOrders2` naming itself as
11+
/// spender and pool, and records the revert data.
12+
contract ReentrantOnTakeOrdersStore {
13+
GenericPoolRaindexV6ArbOrderTaker internal immutable arb;
14+
MockToken internal immutable token;
15+
bytes public revertData;
16+
17+
constructor(GenericPoolRaindexV6ArbOrderTaker arb_, MockToken token_) {
18+
arb = arb_;
19+
token = token_;
20+
}
21+
22+
function set(StateNamespace, bytes32[] calldata) external {
23+
try arb.onTakeOrders2(
24+
address(token),
25+
address(token),
26+
Float.wrap(0),
27+
Float.wrap(0),
28+
abi.encode(address(this), address(this), bytes(""))
29+
) {}
30+
catch (bytes memory err) {
31+
revertData = err;
32+
}
33+
}
34+
35+
function get(bytes32, bytes32) external pure returns (bytes32) {
36+
return 0;
37+
}
38+
39+
fallback() external payable {}
40+
receive() external payable {}
41+
}

0 commit comments

Comments
 (0)