Trust - An attacker can cause temporary freeze of funds to a participant in the FaultDisputeGame

[sherlock-admin3]

Trust

medium

An attacker can cause temporary freeze of funds to a participant in the FaultDisputeGame

Summary

Vulnerability Detail

A fault dispute game is built from the factory, which initializes the first claim in the array below:

claimData.push(
    ClaimData({
        parentIndex: type(uint32).max,
        counteredBy: address(0),
        claimant: tx.origin,
        bond: uint128(msg.value),
        claim: rootClaim(),
        position: ROOT_POSITION,
        clock: LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp)))
    })
);

The sender passes a msg.value which equals the required bond amount, and the registered claimant is tx.origin. At the end of the game, if the claim is honest, the funds will be returned to the claimant.

Suppose the following pre-conditions hold:

• A participant in the game performs an interactions that yields a malicious actor code execution (Simply paying them ETH, or trading an NFT which will reach the receiver's callback, etc)

Now, an attacker can propose an honest root to the DisputeGamseFactory. The registered claimant will be the victim. After a week, their claim will be resolvable, as the game has ended. Upon resolution, the bond will be credited to the victim. _distributeBond(countered == address(0) ? parent.claimant : countered, parent); The issue is that this line represents an unauthorized temporary freeze of funds. The function requests unlocking of the WETH:

WETH.unlock(_recipient, bond);

It appends to a current withdrawal request, and updates the block timestamp:

function unlock(address _guy, uint256 _wad) external {
    // Note that the unlock function can be called by any address, but the actual unlocking capability still only
    // gives the msg.sender the ability to withdraw from the account. As long as the unlock and withdraw functions
    // are called with the proper recipient addresses, this will be safe. Could be made safer by having external
    // accounts execute withdrawals themselves but that would have added extra complexity and made DelayedWETH a
    // leaky abstraction, so we chose this instead.
    WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
    wd.timestamp = block.timestamp;
    wd.amount += _wad;
}

Withdrawal will now be impossible until the DELAY_SECONDS holding time completes: require(wd.timestamp + DELAY_SECONDS <= block.timestamp, "DelayedWETH: withdrawal delay not met");

So, a participant can lose access to any fully unlocked / moments before unlocked funds, for the next full DELAY_SECONDS. This could possibly be chained to keep relocking access to the funds. Temporary freeze of funds is highly dangerous because it contradicts the assumption of the owner that they will be able to mobilize those funds.

For example, if they have taken a loan, and knew the funds would be unlocked in time to repay it (they responsibility worked out the safe loan period), due to this attack they will be liquidated and penalized. This is just one of the many options in which temporary freeze of funds results in monetary losses for the victim.

Impact

An attacker can cause temporary freeze of funds to a participant in the FaultDisputeGame.

Code Snippet

claimData.push(
    ClaimData({
        parentIndex: type(uint32).max,
        counteredBy: address(0),
        claimant: tx.origin,
        bond: uint128(msg.value),
        claim: rootClaim(),
        position: ROOT_POSITION,
        clock: LibClock.wrap(Duration.wrap(0), Timestamp.wrap(uint64(block.timestamp)))
    })
);

Tool used

Manual Review

Recommendation

Consider refactoring the unlocking mechanism - new withdrawal requests should not reset the previous countdowns or issues like the one described could manifest.

[smartcontracts]

Although this report is factually correct, this is the intended behavior. Users can resolve the games for themselves to receive their bonds back without being delayed by an attacker. We are marking as confirmed/wontfix.

[trust1995]

Although this report is factually correct, this is the intended behavior. Users can resolve the games for themselves to receive their bonds back without being delayed by an attacker. We are marking as confirmed/wontfix.

From the reply I suspect the attack vector was understood incorrectly. The idea is that an unrelated transaction between the user and a third-party could be used to inject a claim on behalf of the victim. This claim will eventually be resolvable and will delay cashing out of any already unlocked / pending funds by a week.

[smartcontracts]

I see, this issue is slightly different than the duplicates which just focus on multiple subgames leading to a delay, which is intended. This issue focuses on the tx.origin line which means that a user can be made to be the claimant of the top-level claim. However, withdrawal delays are only relevant within the context of a specific FaultDisputeGame and those delays cannot be used to delay other withdrawals from other FaultDisputeGame contracts. I don't think this actually impacts the user in any way.

[trust1995]

@smartcontracts but unless I'm wrong, attacker could easily submit for the same FaultDisputeGame as the one they want to delay for, so it does affect users (once their own claim is in "pending" phase after resolve, griefer would resolve another claim from same game.

[smartcontracts]

The tx.origin line only applies during initialization, during move the contract uses msg.sender.

[trust1995]

You're 100% right, the vector is invalid. A griefing FDG will always be unrelated to other games.

[smartcontracts]

Yeah I think worst case the user gets nothing, best case the attacker for some reason submits a valid proposal and the user gets a bond for free. Either way we will probably still change the tx.origin thing since it's a bit janky and it's a footgun.

[nevillehuang]

Since no exploit is possible here, and users would still be able to withdraw funds eventually 7 days from last unlock, I believe this issue is low severity, given no core contract functionality is broken and no funds are lost