sherlock-admin2 commented 6 months ago

Trust

high

Theft of initial bonds from proposers who are using smart wallets

Summary

Proposal of output roots through the DisputeGameFactory from Smart Wallets is vulnerable to frontrunning attacks which will steal the initial bond of the proposer.

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.

Smart Wallets are extremely popular ways of holding funds and are used by all types of entities for additional security properties and/or flexibility. A typical smart wallet will receive some execute() call with parameters, verify it's authenticity via signature / multiple signatures, and perform the requested external call. That is how the highly popular Gnosis Safe operates among many others. Smart Wallets are agnostic to whoever actually called the execute() function, as long as the data is authenticated.

These properties as well as the use of tx.origin in the FaultDisputeGame make it easy to steal the bonds of honest proposals:

Scan the mempool for calls to Gnosis execTransaction() or any other variants.
Copy the TX content and call it from the attacker's EOA.
The Smart Wallet will accept the call and send the msg.value to the DisputeGameFactory.
The claimant will now be the attacker.
Upon resolution of the root claim, the attacker will receive the initial bond.

Impact

Theft of funds from an honest victim who did not interact with the system in any wrong way.

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

The Factory needs to pass down the real msg.sender to the FaultDisputeGame.

smartcontracts commented 6 months ago

This report is not entirely correct. It is not possible to "steal" funds from a wallet. Instead, it is the case that the user creating the FaultDisputeGame would not receive their bonds at the end of the game. Although this behavior was intentional as the contracts are meant to be used by EOAs directly and not smart contract wallets, we believe this is a valid low-severity issue and we will fix it.

trust1995 commented 6 months ago

This report is not entirely correct. It is not possible to "steal" funds from a wallet. Instead, it is the case that the user creating the FaultDisputeGame would not receive their bonds at the end of the game.

What you described is essentially stealing - an honest user's bond will be claimed by the attacker.

Although this behavior was intentional as the contracts are meant to be used by EOAs directly and not smart contract wallets, we believe this is a valid low-severity issue and we will fix it.

If the behavior is intentional, then why fix it?

Also, no mention anywhere of the assumption that disputers (which are permissionless) should be EOAs, therefore we can't view that as reducing severity or OOS in any capacity.

smartcontracts commented 6 months ago

I think the implied contract is relatively clear, the user who creates the game is the tx.origin and not the msg.sender. Smart contract wallets weren't an intended user of the contracts. Either way impact is relatively limited (smallest bond size is at the initialization level). I think it's a pretty clear footgun though and should be fixed to prevent issues down the line.

smartcontracts commented 6 months ago

So actual stance here is that this is valid but low-likelihood and low-impact in practice.

sherlock-admin4 commented 6 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/ethereum-optimism/optimism/pull/10149

nevillehuang commented 6 months ago

Based on scope details below, any issue with root cause of the issue stemming from FDG contract will be considered OOS of this contest if airgap and/or delayed WETH mechanism implemented for off-chain review of game results and bond distribution is not shown to be bypassed

https://docs.google.com/document/d/1xjvPwAzD2Zxtx8-P6UE69TuoBwtZPbpwf5zBHAvBJBw/edit

trust1995 commented 6 months ago

@nevillehuang The root cause is clearly in the factory contract using an unusafe tx.origin parameter, as demonstrated in the submission. The finding is in scope.

trust1995 commented 5 months ago

Escalate

The issue is in scope, because:

The bug's origin is certainly not in the FDG's initialize() function - without any changes to the factory there is NO actual way to determine who the correct claimant should be. The FDG does not have the neceesary context, and the root cause is lack of sending the msg.sender of the factory as a parameter. This is further evidenced by the fact the fix changed the Factory's call
The impact is clearly high
Based on the following ruling, the submission must be treated as in-scope: Issues with a root cause in the non-game contracts are IN SCOPE

sherlock-admin2 commented 5 months ago

Escalate

The issue is in scope, because:

The bug's origin is certainly not in the FDG's initialize() function - without any changes to the factory there is NO actual way to determine who the correct claimant should be. The FDG does not have the neceesary context, and the root cause is lack of sending the msg.sender of the factory as a parameter. This is further evidenced by the fact the fix changed the Factory's call

The impact is clearly high

Based on the following ruling, the submission must be treated as in-scope: Issues with a root cause in the non-game contracts are IN SCOPE

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.

nevillehuang commented 5 months ago

Agree that this issue is valid, given the root cause can be seen as stemming from the factory contract. Additionally, there is no mention whether only an EOA is allowed to interact with the contracts. Based on agreed upon scope and line drawn, I believe medium severity to be appropriate since no safety mechanisms (DelyayedWETH) is bypassed.

MightyFox3 commented 5 months ago

Firstly, it's important to clarify that funds cannot be "stolen" from a wallet in the manner described. The scenario involves the user who initiates the FaultDisputeGame; they would not receive their bonds back at the conclusion of the game, which differs significantly from the notion of funds being stolen.

Regarding the vulnerabilities outlined in the original report, it seems there are misconceptions about the ease of exploiting these issues. The steps provided suggest that an attacker can simply scan the mempool, copy transaction content, and execute it from their own externally owned account (EOA). However, this overlooks critical security measures inherent in the system:

The Gnosis smart contract requires signatures from its owners, Alice and Bob, to authorize any execution of the execTransaction() function. This means copying the transaction content and executing it from an attacker's EOA is not feasible unless there is a flaw in how signatures are validated, which is not typical for smart contract wallets, including the Gnosis Safe Wallet.
Furthermore, even if Alice and Bob authorize a transaction, the claim that funds are lost is incorrect. If Alice is the transaction originator and her EOA executes the transaction, she (tx.origin) retains the ability to claim the bond. Therefore, there is no actual loss of funds.

Given these clarifications, it would be more accurate to assess the severity of the issue as low, rather than medium.

trust1995 commented 5 months ago

@MightyFox3 you seem to have completely missed the meat of the exploit, so allow me to re-iterate:

Firstly, it's important to clarify that funds cannot be "stolen" from a wallet in the manner described. The scenario involves the user who initiates the FaultDisputeGame; they would not receive their bonds back at the conclusion of the game, which differs significantly from the notion of funds being stolen.

Respectfully, when an attacker can receive a bond deposited by a victim's account without proofing their claim was invalid, it is considered a theft of funds.

The Gnosis smart contract requires signatures from its owners, Alice and Bob, to authorize any execution of the execTransaction() function. This means copying the transaction content and executing it from an attacker's EOA is not feasible unless there is a flaw in how signatures are validated, which is not typical for smart contract wallets, including the Gnosis Safe Wallet.

Of course it requires signatures, this is the part of the original submission: Copy the TX content and call it from the attacker's EOA. The attacker requires a proposer who is using smart wallet, like the title says. That is not a side exploit or any actual blocking limitation, since we assume the functionality is a valid way of interaction (not otherwise noted).

Furthermore, even if Alice and Bob authorize a transaction, the claim that funds are lost is incorrect. If Alice is the transaction originator and her EOA executes the transaction, she (tx.origin) retains the ability to claim the bond. Therefore, there is no actual loss of funds.

Honestly don't understand the argument - is this saying that if the exploit is botched (doesn't frontrun like it should), it fails? It is shown in the submission that a malicious frontrunner will be registered as the claimant, and receive the bond at the end of the dispute.

MightyFox3 commented 5 months ago

Thank you for clarifying the situation further.

Alice and Bob, who own the smart contract wallet, need to agree and sign off on any transactions that initiate the claim. If Alice submits the transaction and there's no frontrunning interference, she should be the one to claim the bond.

However, if Bob were to submit his own claim before Alice’s is processed—a practice known as frontrunning—he would then be eligible to claim the bond. This could be unfair to Alice, especially if Bob does this deliberately. But such a situation is quite rare since both parties need to agree to initiate the transaction.

This makes the potential problem less severe, as it relies heavily on one party acting against the agreed-upon terms.

54710adk341 commented 5 months ago

You can't just 'front-run' any given smart wallet.

Safe.sol#L141-L161

        {
            if (guard != address(0)) {
                Guard(guard).checkTransaction(
                    // Transaction info
                    to,
                    value,
                    data,
                    operation,
                    safeTxGas,
                    // Payment info
                    baseGas,
                    gasPrice,
                    gasToken,
                    refundReceiver,
                    // Signature info
                    signatures,
                    msg.sender
                );
            }
        }

Smart wallets have guards in place, they check against the msg.sender. Front-running would make execTransaction() fail since the msg.sender would be different.

trust1995 commented 5 months ago

Thank you for clarifying the situation further.

Alice and Bob, who own the smart contract wallet, need to agree and sign off on any transactions that initiate the claim. If Alice submits the transaction and there's no frontrunning interference, she should be the one to claim the bond.

However, if Bob were to submit his own claim before Alice’s is processed—a practice known as frontrunning—he would then be eligible to claim the bond. This could be unfair to Alice, especially if Bob does this deliberately. But such a situation is quite rare since both parties need to agree to initiate the transaction.

This makes the potential problem less severe, as it relies heavily on one party acting against the agreed-upon terms.

No, that's the point. Charlie, an unprivileged attacker who observes the TX Alice sent to the mempool, copies the contents and sends it from his EOA. They frontrun the origin TX and steal the bond.

trust1995 commented 5 months ago

You can't just 'front-run' any given smart wallet.

Safe.sol#L141-L161

        {
            if (guard != address(0)) {
                Guard(guard).checkTransaction(
                    // Transaction info
                    to,
                    value,
                    data,
                    operation,
                    safeTxGas,
                    // Payment info
                    baseGas,
                    gasPrice,
                    gasToken,
                    refundReceiver,
                    // Signature info
                    signatures,
                    msg.sender
                );
            }
        }

Smart wallets have guards in place, they check against the msg.sender. Front-running would make execTransaction() fail since the msg.sender would be different.

That's a wildly incorrect statement. The design of smart wallets is exactly with account abstraction in mind - The TX contents, gas , calldata etc are all signed by the multisig and then anyone can transmit the TX to the blockchain. A TX should be perfectly secure regardless of who is initiating the smart wallet execution call.

The contestant is referring to the optional guard feature, which can perform any type of filtering at the discretion of the multisig. The only two multisigs I've checked, the Chainlink MS and the Optimism MS, don't use any guards. It is, broadly speaking, a mostly unused feature used to perform arbitrary custom validation, and has no relevance to the submission.

lemonmon1984 commented 5 months ago

But at the end, the optimism team can utilize DelayedWETH to address the situation. There is no airgap bypass, and based on the security measures such as DelayedWETH, the funds are secure.

bemic commented 5 months ago

Signatures of Safe owners for a specific transaction are crafted off-chain and passed into the function as input parameters. Once there are enough signatures to pass the threshold, the Safe transaction will be executed. Who is the one who calls the Execute function? It does not matter.

This is the known problem of Safe multisig wallet. There are Guards made specifically to avoid this situation, and they let only one of the owners call the actual execution. However, they are not set up by default. I see this as a known and real problem of Safe. But other protocols like optimism are not forced to be compatible (although they probably should).

Evert0x commented 5 months ago

This issue is either invalid as it flags a design recommendation to mitigate an attack vector. From the perspective of the smart contract it's functioning as normal, it's just that the user didn't take the necessary measures to profit from this.

Or it's valid and Medium as the loss requires specific conditions (smart wallet) and it's constrained as it only applies to the initial bond.

Will revisit this issue

trust1995 commented 5 months ago

From the perspective of the smart contract it's functioning as normal, it's just that the user didn't take the necessary measures to profit from this.

The rationale above can be said about any smart contract exploit, from the perspective of the smart contract, everything is functioning as normal. It's not a design recommendation, because Optimism did not limit interaction with the contracts to only EOAs, and any usage without using a private mempool (extremely likely) is vulnerable.

Or it's valid and Medium as the loss requires specific conditions (smart wallet)

As a C4 Supreme Court judge, that's not the type of conditions that merit lowering a severity. Consider as a thought experiment, a bug that results in loss of funds, only if the first byte of an address is 0xFF. Would that condition reduce severity to Med? Absolutely not, because we realize that over time and considering enough users, it is extremely likely there will be affected users. It is incorrect to look at the single-victim level when the bug is affecting all potential victims.

it's constrained as it only applies to the initial bond.

This argument could also be used if the initial bond is $100000000. Would that make such billion dollar exploits Med? Just to show that saying it is constrained does not cap severities, what matters it the potential concrete impact. There is no respectable judge on the planet that would rule impact of loss of 0.08 ETH = $240 as lower than high.

Evert0x commented 5 months ago

The rationale above can be said about any smart contract exploit, from the perspective of the smart contract, everything is functioning as normal. It's not a design recommendation, because Optimism did not limit interaction with the contracts to only EOAs, and any usage without using a private mempool (extremely likely) is vulnerable.

From the perspective of the protocol's mechanisms it doesn't matter if Alice or Bob executes this transaction. The functionality works as intended as the person executing the transaction will receive the bond. Of course this can't be said about every exploit.

nevillehuang commented 5 months ago

Bonds should belong to the person(s) creating the games that is signing the transaction to create a claim. However, given the permisionless nature of transaction execution for smart wallets as seen here, someone can fron-run and copy the transaction, bypass the transaction checks and act as the tx.origin of that initial proposal of the FDG, receiving that initial bond after resolution. I don't think it should be high severity given the DelayedWETH safety mechanism is not bypassed, so I believe medium severity is appropriate here.

darkbit0 commented 5 months ago

Hey @nevillehuang, Clearly the issue exists in the FaultDisputeGame contract. tx.origin has been used in FaultDisputeGame which its issues were out of scope (unless it bypasses the air-gap). Just because one fix is involving changing the Factory's code doesn't mean the issue exists in out of FaultDisputeGame's code.

Also if any smart wallet allows attackers to front-run its txs, then those smart wallets have vulnerability and the real root cause of this issue is in those smart wallet's code which weren't in the scope of this contest. Users who uses those smart wallets accepted their risk and they also have options to protect their txs and avoid front-runners (by using private mempools or using Guard feature of smart wallet or using smart wallet without front-run issue). There were a lot of similar situations in that past contests that 3rd party systems bugs could effect the protocol and there were fixes for those issues in in-scope Contracts (adding more checks or ...) but those issues were considered as OOS.

trust1995 commented 5 months ago

@darkbit0 It is considered very poor contest etiquette to repeat arguments already discussed. It is showing lack of respect for Watsons and judge's time, and in my opinion should even be punished.

Hey @nevillehuang, Clearly the issue exists in the FaultDisputeGame contract. tx.origin has been used in FaultDisputeGame which its issues were out of scope (unless it bypasses the air-gap). Just because one fix is involving changing the Factory's code doesn't mean the issue exists in out of FaultDisputeGame's code.

Was argued above, and neville's response was:

Agree that this issue is valid, given the root cause can be seen as stemming from the factory contract. Additionally, there is no mention whether only an EOA is allowed to interact with the contracts. Based on agreed upon scope and line drawn, I believe medium severity to be appropriate since no safety mechanisms (DelyayedWETH) is bypassed.

Then:

Also if any smart wallet allows attackers to front-run its txs, then those smart wallets have vulnerability and the real root cause of this issue is in those smart wallet's code which weren't in the scope of this contest. Users who uses those smart wallets accepted their risk and they also have options to protect their txs and avoid front-runners (by using private mempools or using Guard feature of smart wallet or using smart wallet without front-run issue)

This was already explored in depth before your attempt to re-open the discussion.

That's a wildly incorrect statement. The design of smart wallets is exactly with account abstraction in mind - The TX contents, gas , calldata etc are all signed by the multisig and then anyone can transmit the TX to the blockchain. A TX should be perfectly secure regardless of who is initiating the smart wallet execution call.

The contestant is referring to the optional guard feature, which can perform any type of filtering at the discretion of the multisig. The only two multisigs I've checked, the Chainlink MS and the Optimism MS, don't use any guards. It is, broadly speaking, a mostly unused feature used to perform arbitrary custom validation, and has no relevance to the submission.

54710adk341 commented 5 months ago

I think the implied contract is relatively clear, the user who creates the game is the tx.origin and not the msg.sender. Smart contract wallets weren't an intended user of the contracts. Either way impact is relatively limited (smallest bond size is at the initialization level). I think it's a pretty clear footgun though and should be fixed to prevent issues down the line.

This sums it up pretty well, this is a clear footgun, hence this is a valid Low. User errors are not Medium under Sherlock rules.

Evert0x commented 5 months ago

This comment reflects my current stance on this issue https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/194#issuecomment-2094911840

Evert0x commented 5 months ago

Result: Medium Has Duplicates

sherlock-admin2 commented 5 months ago

Escalations have been resolved successfully!

Escalation status:

trust1995: accepted

MightyFox3 commented 5 months ago

https://github.com/sherlock-audit/2024-02-optimism-2024-judging/issues/15

@Evert0x @nevillehuang

trust1995 commented 5 months ago

15

@Evert0x @nevillehuang

??? Has nothing to do with this submission.

sherlock-audit / 2024-02-optimism-2024-judging

Trust - Theft of initial bonds from proposers who are using smart wallets #194

Theft of initial bonds from proposers who are using smart wallets

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

15