LTDingZhen - Users can grief fillers by set malicious `ValidationContract`.

[sherlock-admin2]

LTDingZhen

medium

Users can grief fillers by set malicious ValidationContract.

Summary

When a user creates a new order, he can pass in an additionalValidationContract depending on his preference. When a filler executes the order, it calls the validate function in that contract to do some check. However, the problem is that this ValidationContract can be specified by the creator of the order, making it possible for gas-griefing attack.

Vulnerability Detail

When fillers try to execute a order, order.validate(msg.sender) is called:

function execute(
    SignedOrder calldata order
) external payable override nonReentrant {
    ResolvedOrder[] memory resolvedOrders = new ResolvedOrder[](1);
    resolvedOrders[0] = resolve(order);

    _prepare(resolvedOrders);
    _fill(resolvedOrders);
}

function _prepare(ResolvedOrder[] memory orders) internal {
    uint256 ordersLength = orders.length;
    unchecked {
        for (uint256 i = 0; i < ordersLength; i++) {
            ResolvedOrder memory order = orders[i];
            _injectFees(order);
            order.validate(msg.sender);
            transferInputTokens(order, msg.sender);
        }
    }
}

//@Audit /lib/ResolvedOrderLib.sol
function validate(
    ResolvedOrder memory resolvedOrder,
    address filler
) internal view {
    if (address(this) != address(resolvedOrder.info.reactor)) {
        revert InvalidReactor();
    }

    if (block.timestamp > resolvedOrder.info.deadline) {
        revert DeadlinePassed();
    }

    if (
        address(resolvedOrder.info.additionalValidationContract) !=
        address(0)
    ) {
        resolvedOrder.info.additionalValidationContract.validate(
            filler,
            resolvedOrder
        );
    }
}

Attack path:

1. Attacker makes a tiny swap order, waiting for fillers to take it.
2. Attacker notices the transaction of filler in the mempool.
3. Attacker frongrun the tx, triggers self-destruct and redeploy malicious bytecode on the same address.
4. Filler`s tx gets executed, resulting in a significant loss for the filler.

(Note: such attack doesn't fully rely on frontrun. An attacker can also "guess" the block where the transaction would be executed by offering a good price and make the logic of validate change at that block)

Impact

Fillers may suffer great loss in ETH.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L53-L63

Tool used

Manual Review

Recommendation

Verify if FillerValidation is a trusted address which is controlled by protocol team.

[sherlock-admin]

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

0xAadi commented:

Invalid: OOS

[daoio]

it's an interesting case, but there are 2 inconsistencies here. First, if we are on the chain with a public mempool, it's expected that fillers will relay their txs through a private mempool (at least it's their responsibility to avoid cases as described above); second, if we're on the chain without a public mempool, thus generally frontrunning isn't really possible and responsibilities of fillers shrink to, at least, estimating gas before sending txs and accountining for profitability on their own, therefore it becomes trivial to catch such "order-traps" and just avoid them.

[RealLTDingZhen]

escalate

Hi @cvetanovv @daoio Sorry to disturb, but I believe this is a valid issue.

As I noted above:

(Note: such attack doesn't fully rely on frontrun. An attacker can also "guess" the block where the transaction would be executed by offering a good price and make the logic of validate change at that block)

The trick here is that an attacker can always make fillers execute his order in the first place by offering a good price, and attacker can always guess the block where the trade will be executed.

And, Even if the attacker does not guess the right block--he can just repeat the process at 0 cost, as orders are submitted offchain.

[sherlock-admin2]

escalate

Hi @cvetanovv @daoio Sorry to disturb, but I believe this is a valid issue.

As I noted above:

(Note: such attack doesn't fully rely on frontrun. An attacker can also "guess" the block where the transaction would be executed by offering a good price and make the logic of validate change at that block)

The trick here is that an attacker can always make fillers execute his order in the first place by offering a good price, and attacker can always guess the block where the trade will be executed.

And, Even if the attacker does not guess the right block--he can just repeat the process at 0 cost, as orders are submitted offchain.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ArnieSec]

Should remain invalid. The filler can view the validation contract logic and see the self destruct function and avoid it.

Additionally metamorphic contracts require at the very least 2 txs. So the frontrun/ guessing is basically impossible.

The upgrade process will not be as smooth for metamorphic contracts, as selfdestruct operations are recorded in the transaction substate and performed at the end of a transaction. This means that an upgrade will require two transactions, and so the contract code will be empty (and susceptible to intermediate usage) between the two transactions

This is described in a medium article by 0age

https://0age.medium.com/the-promise-and-the-peril-of-metamorphic-contracts-9eb8b8413c5e

[RealLTDingZhen]

1. This issue does not rely on selfdestruct as well. An attacker can also set up a upgradeable proxy and have the contract upgrade at the right block.
2. It is not actually necessary to execute the logic at the very least 2 txs. Executing the attack logic at an earlier position within the same block would work. As I said above, Due to competition between different fillers, attackers can always estimate the block range in which the transaction will be executed. Even if the guess is wrong, the attacker doesn't lose any gas

[ArnieSec]

@RealLTDingZhen

From Sherlock docs

In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

Low severity.

Your issue describes an impractical use of parameters to reach OOG(out of gas) state.

[ArnieSec]

Additionally, we are judging the severity of this issue based on out of scope contracts. This already makes the issue invalid.

From uniswapX docs which this project is forked from

It’s up to the individual filler to architect their own systems for finding and executing profitable orders.

Also this

If the order is valid, it will be competing against other fillers attempts to execute it in a gas auction. For this reason, we recommend submitting these transactions through a service like Flashbots Protect.

It's up to the fillers to judge the risk of an order. This issue is not valid.

Link to uniswapX docs

https://docs.uniswap.org/contracts/uniswapx/guides/createfiller

[RealLTDingZhen]

Out of Gas: Issues that result in Out of Gas errors either by the malicious user filling up the arrays or there is a practical call flow that results in OOG can be considered a valid medium or in cases of blocking all user funds forever maybe a valid high. Exception: In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

There is a practical call flow. Frontrunning and speculation are both viable. Even if fillers choose a private pool, the order will be executed within one or two blocks after lastExclusiveTimestamp. Attacker can just send the attack tx into the same private pool and expect it to get executed earlier in the same block.

Contract Scope: If a contract is in contest Scope, then all its parent contracts are included by default. In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue. If there is a vulnerability in a contract from the contest repository but is not included in the scope then issues related to it cannot be considered valid.

In-scope contracts uses the library.

[ArnieSec]

@RealLTDingZhen regardless the system was designed in a way where the risk is entirely up to the filler. The filler must measure the risk by reviewing parameters. These parameters include the validation contract. Which once he sees its upgradeable, he can just avoid filling the order.

From documentation of uniswapX

Trust Assumptions The current version of the UniswapX RFQ system operates through a closed vetted group of quoters, who are not expected to misbehave. This helps maintain a good swapper experience and prevent any potential misuse of the system. Users trust the RFQ system to provide them with sensible parameters for their orders. Swappers and fillers trust the Uniswap API, where orders are exchanged, that all information is accurate and no malicious data can be injected by any party.

While I see your points, this issue is still clearly invalid.

[RealLTDingZhen]

Hi @ArnieGod I still don't get your point.

1. Rubicon protocol is a derivative work of UniswapX. There is no indication in the official documentation provided during the audit that the system was designed in a way where the risk is entirely up to the filler.
2. The issue here is that filler actually cannot identify the attack before the attack tx, so I believe actually its protocol`s duty to protect fillers from such attack.

[ArnieSec]

From rubicon readMe

Add links to relevant protocol resources

Gladius intergration guide - https://rubicondefi.notion.site/Rubicon-Gladius-Integration-Guide-ee55cbe3ccdf4b88a6780d8ef090599f

UniswapX documentation - https://docs.uniswap.org/contracts/uniswapx/overview

The uniswapX docs are deemed relevant resources. So therefore anything stated there should be relevant to this contest.

Additionally

From rubicon readme

GladiusReactor is based on UniswapX’s ExclusiveDutchOrderReactor and intended to support ExclusiveDutchOrders.

Changing validation may cause intended support to not work.

[ArnieSec]

Let's leave it up to the judge for now, it's ultimately up to him. Good discussion @RealLTDingZhen I always like a good debate 👍

[daoio]

A couple of notes from my side here. First, as mentioned, it's the full responsibility of a filler to analyze orders from an off-chain order pool. This particular "trap" can be caught in multiple ways, which are relatively easy to implement on the filler's side:

• If we're dealing with an upgrade, inspect the bytecode of the additionalValidationContract to find a DELEGATECALL opcode - it should be relatively easy to differentiate from random occurrences of F4s. The presence of this opcode alone should raise enough suspicion to avoid executing the particular order.
• Inspect the selector table. If there's no selector for getFeeOutputs(), the filler might assume it's a proxy that will delegatecall to the implementation, and can avoid executing this particular order.
• Capture the implementation's address (either in storage or by following DELEGATECALL arguments), and pass its address and the keccak256 hash of its bytecode as a callbackData. Then, verify that the hash of the bytecode of the implementation during execution is the same as it was before.
• Execute a low-level call to the validate function of an additionalValidationContract (which can be fetched either from the encoded order or passed as additional callbackData) with a fixed gasLimit (e.g., 500k). This assumes that since it's just a view function, it won't consume a significant amount of gas. In the case of a legitimate contract, any unused gas will be refunded. However, in the case of a malicious contract, the call will revert with OOG, what can be detected by checking the result of the low-level call.

Second, if executing such an attack requires a transaction from the swapper's side to upgrade or selfdestruct the contract, it would not be economically reasonable for them to do so, because executing this attack would entail spending ETH, with the sole outcome being the potential loss of ETH from the filler (and ETH loss from the attacker) without gaining anything in return, thus it's not a zero cost, but rather an attacker will always spend ETH on gas.

[RealLTDingZhen]

Hi @daoio The problem is that this kind of attack can always be unforeseen. Even if filler does all the checks above, a simple if-else can bypass it. The attacker only needs to change the validate logic between simulated and real transactions. Consider such logic:

function validate(
    address filler,
    ResolvedOrder calldata resolvedOrder
) external {

    if (changed) {
        normal logic
    }
    else {
        evil logic
    }
}

function changelogic() external onlyattacker{
    changed = 0;
}

And, this vulnerability can be easily fixed from the protocol-side:

    bool success;
    bytes memory result;
    (success, result) = address(validator).call{gas: 100000}(abi.encodeWithSignature("validate(address,ResolvedOrder)", param);

Or uses try-catch to ignore OOG revert, which enables other orders in the same transaction to be executed normally

[ArnieSec]

because executing this attack would entail spending ETH, with the sole outcome being the potential loss of ETH from the filler (and ETH loss from the attacker) without gaining anything in return, thus it's not a zero cost, but rather an attacker will always spend ETH on gas.

Attacker will spend eth on gas to call changeLogic, and he will get nothing in return @RealLTDingZhen

This is griefing

From Sherlock docs

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Since the function we are calling is not time sensitive. This issue is not valid.

[RealLTDingZhen]

1. For filler without proper protection, the filler will lose a lot of ETH. Attackers can also get fillers to perform some gas-consuming operations for them. Malicious fillers can also abuse this to make it easier for themselves to execute certain high-profit trades by DoS other fillers.
2. the function we are calling is time sensitive. Consider orders with tight deadline and decayEndTime. Even if fillers manage to execute the same order in the next block after the attack, it will be different exchange ratio they get. As fillers are encouraged to fulfil many orders at the same time, they will subject to more losses.

[ArnieSec]

This is too circumstantial,1. The filler must not be aware of the malicious contract 2. The attacker must guess perfectly which block to call changeLogic on because it's recommended in docs that fillers use flashbots protect.

3. The attackers tx must now frontrun the victims tx in order for the change to go into effect. 4. We are assuming that the victim fillers always pick the malicious orders instead of picking the good ones before the attacker. 5. Once again the risk must be assessed by the filler and it is up to him to pick correct orders

The function is time sensitive but it doesn't matter because it will be executed on the same block that the filler calls the tx. And if it fails that means that the malicious seller added logic to ensure an OOG (out of gas). This means the malicious seller never intended his order to get filled, therefore it is not time sensitive.

There's so many moving parts that have to be true for this issue to even happen... and in the end the filler is responsible for his own choice of which orders to fill anyway.

[RealLTDingZhen]

I've shown above that all five points are perfectly feasible and require no special assumptions. Even if it fails, the gas to change a state variable is very low. Attacker can simply reprocess the attack.

Fillers are bots. They judge which trades they should execute based on profits.

And, I don't think filler is responsible for his own choice is a suitable reason to invalidate this issue. If we considers fillers to be normal users, this attack is not based on users‘ mistake, so I don't understand why this is a problem.

[RealLTDingZhen]

I think I've made all my points. I will let @Czar102 and @cvetanovv to decide.

[mstpr]

This should be invalid per Sherlock rules. Fillers are not forced to solve orders of swappers. Fillers also doesn't need to send infinite amount of gas to solve it. They can send the viable amount of gas and at max this would revert. Tho, a filler seeing that a weird validation contract used can and should avoid to fill this order anyways

[RealLTDingZhen]

Filler is a BOT not strong AI. I don't think fillers can distinguish between weird validation contract and normal validation contract

[mstpr]

if (validationContract == something I trust or address(0) { fill the order }

if (validationContract == something I dont trust but looks like a good trade) { fill the order with reasonable gas sent, so I dont lose too much gas by calling it if the validation contract tricks me or, dont fill at all. }

[RealLTDingZhen]

That doesn't change the fact that the attack was objectively feasible. Even though some users(fillers) can mitigate/avoid this issue through their own measures, this vulnerability still exists in the contract and can be abused against other fillers. That shouldn't be the basis for judging the question invalid.

Out of Gas: Issues that result in Out of Gas errors either by the malicious user filling up the arrays or there is a practical call flow that results in OOG can be considered a valid medium or in cases of blocking all user funds forever maybe a valid high. Exception: In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue: The issue causes locking of funds for users for more than a week. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

I still think this issue should be judged as valid med according to the doc above.

[mstpr]

That doesn't change the fact that the attack was objectively feasible. Even though some users(fillers) can mitigate/avoid this issue through their own measures, this vulnerability still exists in the contract and can be abused against other fillers. That shouldn't be the basis for judging the question invalid.

Out of Gas: Issues that result in Out of Gas errors either by the malicious user filling up the arrays or there is a practical call flow that results in OOG can be considered a valid medium or in cases of blocking all user funds forever maybe a valid high. Exception: In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue: The issue causes locking of funds for users for more than a week. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

I still think this issue should be judged as valid med according to the doc above.

I don't see any "attack" here. At most there is a "bait". Fillers will resolve the orders by sending considerable amount of gas, not infinite. Fillers also shouldn't solve any orders that they think the validation address looks weird

[RealLTDingZhen]

Its a DoS/griefing on fillers.

Despite of the gas loss, all orders in the bundle tx cannot be made. Such bundle is time-sensitive.

[ArnieSec]

I think your issue has evolved so far out of the original report.

Your original report states self destruct to get new byte code on contract. The original impact was loss of eth via gas.

When the above was disputed then your issue changed to a proxy implementation to trick the fillers.

When that was then disputed, you introduced an else if that can have different outcomes depending on a function call to change state.

Now impact has changed once again to DOS.

Respectfully this alone should make it invalid, not to mention that if all of this was included in the original report, it would still be invalid.

[RealLTDingZhen]

This is because my original REPORT was based on the fact that fillers are bots that only have simple logic(based on profit) and don't submit transactions through private pools(like uniswapX). The discussion that followed was only because you wanted to make filler a strong AI and I'm trying to explain how this attack was never foreseen by the filler.

grief in a block = temporary DoS, That's what the sherlock doc says.

[ArnieSec]

No that's not what Sherlock docs say.

Sherlock docs

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block

Let's highlight this specific part

frontrunning a transaction to fail, even if can be done perpetually

Front running a tx to fail is considered a dos...

Let's look at your original report

Filler`s tx gets executed, resulting in a significant loss for the filler.

Fillers may suffer great loss in ETH.

A dos/ failing tx was never mentioned in your original report, you even state the fillers tx gets executed.

[Czar102]

I think there has been a lot of excessive discussion here.

Fillers are not forced to solve orders of swappers.

If this is true, I don't see an issue here. If a solver is smart enough to think they can fill orders for arbitrary validation contracts, this is not forbidden. If not, they are free not to consider that order for filling. See https://github.com/sherlock-audit/2024-02-rubicon-finance-judging/issues/53#issuecomment-1946129477.

Planning to reject the escalation and leave the issue as is.

[RealLTDingZhen]

I dont think

Fillers are not forced to solve orders of swappers.

hits the point.

Fillers are protocol users as well. Even though some fillers can mitigate it by operating on their own logic, other users may also suffer from it.

For example, on those no-slippage-protection issues, some users can set their own slippage logic in a contract instead of using an EOA, but that doesn't change the fact that there is no slippage protection for normal users.

[Czar102]

Result: Invalid Unique

Fillers should ignore an order if they can experience any loss due to an implementation of the validation contract being not fully understood by them.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• RealLTDingZhen: rejected