From 5780cd562bb16be2615ce795f8909885874b6641 Mon Sep 17 00:00:00 2001 From: dev-aly3n Date: Thu, 6 Aug 2026 16:37:22 +0400 Subject: [PATCH] fix: revert when msg.value does not cover a native fill _fill's native branch sends outputAmount to the recipient, but nothing requires msg.value to cover it. _refundNativeExcess handles only the excess case, so a shortfall is paid out of address(this).balance. The check is placed in _refundNativeExcess rather than in _fill because fillOrderOutputs accumulates totalNativeSent across outputs and settles once; a per-output check would pass on each individual output while the total overdrew. Both entry points already funnel through this helper. Adds a batch test for the cumulative overdraw case and one asserting a correctly funded fill still refunds its excess and leaves a residual balance untouched. --- src/output/OutputSettlerBase.sol | 8 +- test/output/OutputSettlerSimple.fill.t.sol | 39 ++++++ ...OutputSettlerSimple.fillOrderOutputs.t.sol | 111 ++++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) diff --git a/src/output/OutputSettlerBase.sol b/src/output/OutputSettlerBase.sol index f681f43..49f3023 100644 --- a/src/output/OutputSettlerBase.sol +++ b/src/output/OutputSettlerBase.sol @@ -87,6 +87,8 @@ abstract contract OutputSettlerBase is IAttester, BaseInputOracle { error PayloadTooSmall(); /// @dev Payload does not lead with a known domain magic error InvalidPayloadMagic(bytes4 magic); + /// @dev `msg.value` does not cover the native output(s) paid out by the fill + error InsufficientNativeValue(uint256 required, uint256 provided); /** * @notice Sets outputs as filled by their solver identifier, such that outputs won't be filled twice. @@ -288,12 +290,16 @@ abstract contract OutputSettlerBase is IAttester, BaseInputOracle { } /** - * @notice Refunds the unused native value to msg.sender. + * @notice Checks that `msg.value` covers the native paid out, and refunds the unused remainder to msg.sender. + * @dev The coverage check lives here rather than in `_fill` because `fillOrderOutputs` accumulates `nativeSent` + * across every output and settles once. A per-output check would pass on each individual output while the total + * overdrew the contract's balance. * @param nativeSent Amount of native already paid out by `_fill`. */ function _refundNativeExcess( uint256 nativeSent ) internal { + if (msg.value < nativeSent) revert InsufficientNativeValue(nativeSent, msg.value); if (msg.value > nativeSent) Address.sendValue(payable(msg.sender), msg.value - nativeSent); } diff --git a/test/output/OutputSettlerSimple.fill.t.sol b/test/output/OutputSettlerSimple.fill.t.sol index 36f7d6a..0381039 100644 --- a/test/output/OutputSettlerSimple.fill.t.sol +++ b/test/output/OutputSettlerSimple.fill.t.sol @@ -962,6 +962,45 @@ contract OutputSettlerSimpleTestFill is Test { assertEq(sender.balance, senderBalanceBeforeSecond); } + /// @notice A native fill must revert when `msg.value` does not cover the output, rather than paying it out of a + /// residual balance held by the settler. + /// @dev The settler is not meant to hold a balance, but it is not prevented from receiving one (for example via a + /// SELFDESTRUCT force-feed). Without the coverage check, a self-order with a native output and `msg.value = 0` + /// would let anyone sweep that balance. + function test_fill_native_token_insufficient_value_with_residual_balance( + bytes32 orderId, + bytes32 filler, + uint128 amount + ) public { + vm.assume(filler != bytes32(0) && amount > 0); + + address sender = makeAddr("sender"); + // Residual balance on the settler, as a SELFDESTRUCT force-feed could produce. + vm.deal(outputSettlerCoinAddress, amount); + + bytes memory fillerData = abi.encodePacked(filler); + + // A self-order: the caller is also the recipient of the native output. + MandateOutput memory outputStruct = MandateOutput({ + oracle: bytes32(0), + settler: bytes32(uint256(uint160(outputSettlerCoinAddress))), + chainId: block.chainid, + token: bytes32(0), + amount: amount, + recipient: bytes32(uint256(uint160(sender))), + callbackData: bytes(""), + context: bytes("") + }); + + vm.prank(sender); + vm.expectRevert(abi.encodeWithSignature("InsufficientNativeValue(uint256,uint256)", uint256(amount), 0)); + outputSettlerCoin.fill{ value: 0 }(orderId, outputStruct, type(uint48).max, fillerData); + + // The settler's balance was not swept. + assertEq(outputSettlerCoinAddress.balance, uint256(amount)); + assertEq(sender.balance, 0); + } + function test_fill_native_token_with_callback( bytes32 orderId, uint256 amount, diff --git a/test/output/OutputSettlerSimple.fillOrderOutputs.t.sol b/test/output/OutputSettlerSimple.fillOrderOutputs.t.sol index 2eb88cf..cb740de 100644 --- a/test/output/OutputSettlerSimple.fillOrderOutputs.t.sol +++ b/test/output/OutputSettlerSimple.fillOrderOutputs.t.sol @@ -416,6 +416,117 @@ contract OutputSettlerSimpleTestfillOrderOutputs is Test { outputSettlerCoin.fillOrderOutputs{ value: sentValue }(orderId, outputs, type(uint48).max, fillerData); } + /// @notice A batch whose native outputs sum to more than `msg.value` must revert, even when the settler holds a + /// residual balance large enough to cover the difference. + /// @dev This is the case that a per-output check inside `_fill` would miss: `msg.value` covers each individual + /// output, but not their sum. Only the cumulative check in `_refundNativeExcess` catches it. + function test_fill_batch_native_token_total_exceeds_value( + bytes32 orderId, + bytes32 filler, + uint128 amount1, + uint128 amount2 + ) public { + vm.assume(filler != bytes32(0)); + vm.assume(amount1 > 0 && amount2 > 0 && amount1 != amount2); + + address sender = makeAddr("sender"); + uint256 totalRequired = uint256(amount1) + uint256(amount2); + // Enough to cover either output on its own, but not both together. + uint256 sentValue = amount1 > amount2 ? uint256(amount1) : uint256(amount2); + vm.deal(sender, sentValue); + // Residual balance on the settler, as a SELFDESTRUCT force-feed could produce. Without it the second + // `Address.sendValue` would revert on an empty balance and mask the overdraw. + vm.deal(outputSettlerCoinAddress, totalRequired); + + MandateOutput[] memory outputs = new MandateOutput[](2); + + outputs[0] = MandateOutput({ + oracle: bytes32(0), + settler: bytes32(uint256(uint160(outputSettlerCoinAddress))), + chainId: block.chainid, + token: bytes32(0), // native token + amount: amount1, + recipient: bytes32(uint256(uint160(swapper))), + callbackData: bytes(""), + context: bytes("") + }); + + outputs[1] = MandateOutput({ + oracle: bytes32(0), + settler: bytes32(uint256(uint160(outputSettlerCoinAddress))), + chainId: block.chainid, + token: bytes32(0), // native token + amount: amount2, + recipient: bytes32(uint256(uint160(swapper))), + callbackData: bytes(""), + context: bytes("") + }); + + bytes memory fillerData = abi.encodePacked(filler); + + vm.prank(sender); + vm.expectRevert(abi.encodeWithSignature("InsufficientNativeValue(uint256,uint256)", totalRequired, sentValue)); + outputSettlerCoin.fillOrderOutputs{ value: sentValue }(orderId, outputs, type(uint48).max, fillerData); + + // The revert unwound both sends, so the residual balance is intact. + assertEq(outputSettlerCoinAddress.balance, totalRequired); + } + + /// @notice A correctly funded batch still fills and still refunds the excess, and leaves any residual balance + /// held by the settler untouched. + function test_fill_batch_native_token_excess_refund_leaves_residual( + bytes32 orderId, + bytes32 filler, + uint128 amount1, + uint128 amount2, + uint128 excess, + uint128 residual + ) public { + vm.assume(filler != bytes32(0)); + vm.assume(amount1 > 0 && amount2 > 0 && amount1 != amount2); + + address sender = makeAddr("sender"); + uint256 totalRequired = uint256(amount1) + uint256(amount2); + uint256 totalSent = totalRequired + uint256(excess); + vm.deal(sender, totalSent); + vm.deal(outputSettlerCoinAddress, residual); + + MandateOutput[] memory outputs = new MandateOutput[](2); + + outputs[0] = MandateOutput({ + oracle: bytes32(0), + settler: bytes32(uint256(uint160(outputSettlerCoinAddress))), + chainId: block.chainid, + token: bytes32(0), // native token + amount: amount1, + recipient: bytes32(uint256(uint160(swapper))), + callbackData: bytes(""), + context: bytes("") + }); + + outputs[1] = MandateOutput({ + oracle: bytes32(0), + settler: bytes32(uint256(uint160(outputSettlerCoinAddress))), + chainId: block.chainid, + token: bytes32(0), // native token + amount: amount2, + recipient: bytes32(uint256(uint160(swapper))), + callbackData: bytes(""), + context: bytes("") + }); + + bytes memory fillerData = abi.encodePacked(filler); + + uint256 swapperBalanceBefore = swapper.balance; + + vm.prank(sender); + outputSettlerCoin.fillOrderOutputs{ value: totalSent }(orderId, outputs, type(uint48).max, fillerData); + + assertEq(swapper.balance, swapperBalanceBefore + totalRequired); + assertEq(sender.balance, uint256(excess)); // Excess refunded + assertEq(outputSettlerCoinAddress.balance, uint256(residual)); // Residual untouched + } + /// @notice Recipient of the first (native) output reenters `fill` while the batch fills a subsequent ERC20 output. function test_fill_batch_native_then_erc20_excess_refund_with_reentering_recipient() public { bytes32 orderId = keccak256(bytes("orderId"));