sherlock-admin4 commented 5 months ago

Trust

medium

M - The safety mechanism of the DelayedWETH contract can be bypassed

Summary

A malicious user can frontrun directly the admin's call of hold() function in DelayedWETH. They can reset back the approval to zero, so that the admin cannot actually confiscate their funds.

Vulnerability Detail

DelayedWETH has several safety mechanisms in place to prevent malicious actors from cashing out illicit profits. The hold() function allows the owner to set arbitrary approval:

function hold(address _guy, uint256 _wad) external {
    require(msg.sender == owner(), "DelayedWETH: not owner");
    allowance[_guy][msg.sender] = _wad;
    emit Approval(_guy, msg.sender, _wad);
}

The function only sets a custom approval for the admin. However it doesn't make use of that approval. A malicious guy who is supposed to lose access to their funds, can frontrun the call with approve(admin, 0). At this point, the admin can no longer confiscate their funds.

The only resolution for the admin is to bundle together the hold() and transferFrom() calls, but until they figure out how to do that, additional time was wasted, making it likely that the freezing time will have passed and the malicious user can call withdraw() to cash out their profits into ETH, which is unsanctionable.

Impact

Bypass of a key security mechanism of the DelayedWETH contract

Code Snippet

function hold(address _guy, uint256 _wad) external {
    require(msg.sender == owner(), "DelayedWETH: not owner");
    allowance[_guy][msg.sender] = _wad;
    emit Approval(_guy, msg.sender, _wad);
}

Tool used

Manual Review

Recommendation

The hold() function should either:

Disable the approve() functionality from the malicious user
Actually seize the funds inside the hold() function.

smartcontracts commented 4 months ago

Although this report is factually correct, this is the intended behavior. The contract is primarily designed to be able to hold funds from the FaultDisputeGame contract which does not call the transfer function. Additionally, the owner could also execute hold and transferFrom within the same transaction bundle to prevent this vector. Marking as confirmed/wontfix.

nevillehuang commented 4 months ago

@trust1995 Based on similar reasonings highlighted previously seen here supporting sponsors argument, I believe this issue is low severity.

trust1995 commented 4 months ago

@nevillehuang Would like to consider the following arguments:

No evidence to support owner will call both functions together
First time owner finds out about resetting approval attack, it will be a ticking time bomb. Whether they will have enough time to bundle and execute them together before the timer runs out is up for debate. This is a circumstance specific to the Optimism codebase.
Regardless, I view dependence on flashbots to save the day in order to reduce severity from M -> L invalid (H -> M is valid). But that's something that can be discussed separately.

trust1995 commented 4 months ago

Escalate

I believe the finding to be in scope by the reasoning below:

No evidence to support owner will call both functions together
First time owner finds out about resetting approval attack, it will be a ticking time bomb. Whether they will have enough time to bundle and execute them together before the timer runs out is up for debate. This is a circumstance specific to the Optimism codebase.
Regardless, I view dependence on flashbots to save the day in order to reduce severity from M -> L invalid (H -> M is valid). Flashbot represents a significant trust assumption and is far from a holy grail when it comes to exploit protection. Without assumption of guarantee of bundling, there is clearly no way the owner can guarantee safe confiscation of the funds.
The submission can be viewed as a valid breaking of the airgap assumptions (owner does not actually have the airgap equivalent amount of time to address a wrongly resolved claim).

sherlock-admin2 commented 4 months ago

Escalate

I believe the finding to be in scope by the reasoning below:

No evidence to support owner will call both functions together

First time owner finds out about resetting approval attack, it will be a ticking time bomb. Whether they will have enough time to bundle and execute them together before the timer runs out is up for debate. This is a circumstance specific to the Optimism codebase.

Regardless, I view dependence on flashbots to save the day in order to reduce severity from M -> L invalid (H -> M is valid). Flashbot represents a significant trust assumption and is far from a holy grail when it comes to exploit protection. Without assumption of guarantee of bundling, there is clearly no way the owner can guarantee safe confiscation of the funds.

The submission can be viewed as a valid breaking of the airgap assumptions (owner does not actually have the airgap equivalent amount of time to address a wrongly resolved claim).

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 4 months ago

This issue depends on if flashbots/tx bundling are an appropriate solution to avoid front-running. If no, this can be a valid medium because it allows users to potentially keep bypassing the DelayedWETH safety mechanism by overriding allowance, but they would have to consistently front-run the admin executing this functions. A single successful bundled tx on mainnet would make this impossible, but is dependent on availability/trust on flashbots/bundling services. Per my comment here the use of flashbots were historically considered.

Evert0x commented 4 months ago

I believe the following language applies to this issue, as it's (perpetual) DoS attack of the admin action.

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.

@trust1995 can you expand on the time sensitiveness of this issue? What's the impact if the time runs out?

First time owner finds out about resetting approval attack, it will be a ticking time bomb. Whether they will have enough time to bundle and execute them together before the timer runs out is up for debate.

trust1995 commented 4 months ago

Yeah, so in Optimism a core safety mechanism is that bonds cannot be claimed from DelayedWETH until the DELAY has passed. Assuming that games can resolve incorrectly, the DELAY is preventing attackers from running with the money. As soon as the DELAY expires, attacker can cash out their illegal profits.

The assumption that owner has DELAY amount of time to undo the hack is one of the core safety steps Optimism was interested in verifying in the contest.

Evert0x commented 4 months ago

With that context I believe Medium severity is appropriate. Planning to accept escalation and assign Medium

darkbit0 commented 4 months ago

Hi @Evert0x , This attack would only be possible if Guardian can't perform his action in the DELAY_SECONDS period (which is set by admins in DelayedWETH). This is from contest page:

The Guardian is assumed to be honest and responsive with an SLA of 24 hours.

Guardian is trusted to operate in 24 hours and delays in the DelayedWETH will be set based on this value(24 hours) so for sure Gurdian would have enough time to perform and they aren't gonna wait for last seconds to perform their actions. We can't consider this function as time-sensitive (If Guardian operates in rational times, then delaying 5-10 min doesn't gonna change function's correctness)

The attack would be possible if attacker could DOS the Guardian for the whole duration of the DELAY_SECONDS in DelayedWETH which can't be done because Guardian can use flash-bots or private mempool after the first attack(front-running) attempts. So in summery attacker can delay Guardian for 1-2 block but Guardian has 24 hours to perform his action. I don't believe this attack vector has "time-sensitive" property that rules says.

This attack vector won't be possible if admins used flash-bots or private mempools. In previous contests @Czar102 marked this type of issues as Low as @nevillehuang mentioned. link1 link2

I think griefing is not sufficient of an impact to be considered H/M according to Sherlock rules. Please note that frontrunning to get the transaction to fail is not a 1 year DoS, but a 1-2 block DoS. Doing that multiple times doesn't forbid the griefed user to bundle the transactions they want to make. The protocol will be on mainnet, so ways to bundle transactions exist (Flashbots, for example).

I understand the attack path, my point was that repetitively DoSing for a single transaction (griefing) is not considered a DoS for more than one year. Someone can simply use a private mempool to get their transaction accepted at any point during that time.

So clearly based on previous judging front-running is only accepted as 1-2 block delay and it would be considered as valid attack if delaying 1-2 block would cause damage. Judging rules confirms it and states that front-running attack would be valid DOS/Greif if postponing the transaction for one block("time-sensitive") could cause damage which isn't the case here.

trust1995 commented 4 months ago

No, I believe Evert0X understood the time-sensitive nature correctly. The core assumption Opt is working with is that they have 7 days to confiscate the funds. Suppose they call hold() on day X, < 7, and transfer out the funds on day Y, where X < Y < 7. At that moment, attacker frontruns and resets approval. From that moment, Optimism has 7 - Y to:

figure out the confiscation failed
prepare a bundling script and operation
obtain access to a private mempool
execute the bundle through the private mempool

Clearly the above constraints represent a significant deviation from the intended air-gap model, and Optimism explicitly stated the importance of that model being bulletproof. Therefore, H/M is justified.

bizzyvinci commented 4 months ago

I believe wrong assumptions are made about DelayedWETH. guy is FDG, not user.

DelayedWETH is owned and held by FDG and users are expected to interact with FDG only. When they play games, they send ETH to FDG. Then FDG deposits the ETH into DelayedWETH here and here.

Therefore, user has 0 WETH, but FDG owns the WETH. When it's time to withdraw, it would call unlock here. unlock does not update balance. When it's time to claim, FDG withdraws and transfer ETH to user here.

The WETH balance of user is 0 throughout the game and lock period. And I guess that's what @smartcontracts meant by "The contract is primarily designed to be able to hold funds from the FaultDisputeGame contract which does not call the transfer function"

54710adk341 commented 4 months ago

It is baffling how invalid issues can be escalated into valid issues just because of the clout that a person has. Absolutely crazy.

guhu95 commented 4 months ago

@bizzyvinci is correct, and the finding is invalid: only the the game contract ever holds and uses DelayedWETH, so it can't call approve maliciously, so no frontrunning is possible.

This also explains the sponsor's "intended behavior" + "wont-fix" resolution, which wouldn't make sense otherwise.

nevillehuang commented 4 months ago

I might have some misunderstanding of how DelayedWETH is supposed to work if thats the case. If I am understanding correctly,

Funds unlocked after game resolution, initiating 7 day delay on FDG contract
Off-chain mechanism is presumed to detect any wrong bond assignments during this time
Hence, the hold function is only ever intended to be used on FDG contract before bonds are withdrawn by wrongful recipient

Hmm so for this issue to be valid, the assumption here is that the off-chain mechanism is bypassed and after the 7 day delay, the attacker would have wrongfully withdrawn bonds, and then thereafter the admin would have to call hold to confiscate funds? If so this issue seems invalid.

guhu95 commented 4 months ago

@nevillehuang your comment seems mostly correct, but just to clarify about this description:

Hmm so for this issue to be valid, the assumption here is that the off-chain mechanism is bypassed and after the 7 day delay, the attacker would have wrongfully withdrawn bonds, and then thereafter the admin would have to call hold to confiscate funds? If so this issue seems invalid.

Calling hold is only needed (as a safeguard) during the 7 day period when FDG is the holder - between its calls to DelayedWETH.unlock and DelayedWETH.withdraw. After that, DelayedWETH is no longer relevant, since native ETH is sent to the user.

This flow is the intended flow, and frontrunning is not possible in it (because FDG is the holder of DelayedWETH).

The previous confusion here is likely due to DelayedWETH never being used as a token (it's never transferred).

trust1995 commented 4 months ago

The discussion above outlined the fact that the FaultDisputeGame contract is the one technically holding DelayedWETH and hence a participant in the game cannot directly call approve(0) to reset the approval.

However the point of the submission is a flaw rooted in DelayedWETH, not in how it is integrated with FaultDisputeGame! Nowhere is it mention that only mis-integrations with FDG are in-scope. The issue outlined is correct in the general case of DelayedWETH usage, through use by EOAs or any future games, etc.

All the discussion around ticking time-bomb, 7-Y time to respond instead of 7, etc, are still correct in the general scope of DelayedWETH, and should be considered in-scope (outside the FDG resolution logic).

0xlastByte commented 4 months ago

@trust1995 agree and this comment confirms https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol#L19-L21 it says: owner address that can pull WETH from any -> account <- and can recover ETH from the contract itself

bizzyvinci commented 4 months ago

@0xlastByte You left out this comment

DelayedWETH is an extension to WETH9 that allows for delayed withdrawals

And this is just right above what you referenced.

DelayedWETH is designed to be used by the DisputeGame contracts where unlock will only be triggered after a dispute is resolved.

And this is an excerpt from the contest README

DelayedWETH includes a mapping that associates attempts to withdraw bonded ETH with specific recipient addresses. However, these specific recipient addresses are not actually required to participate in the process of claiming these withdrawals from the DelayedWETH contract. The FaultDisputeGame is written to be restrained in such a way that this "soft" accounting (no real restriction that forces users to make use of the accounting system) is actually "hard" accounting and cannot be circumvented by users (other than by a bug in the FaultDisputeGame). This decision was made so that users would not have to interact with the DelayedWETH contract directly and the contract could therefore be removed at a later date without impacting challenger software.

Why would any user deposit to DelayedWETH instead of normal WETH? What can the user do to be labelled as malicious and motivate Optimism to hold the WETH?

trust1995 commented 4 months ago

@bizzyvinci Your excerpts point to DelayedWETH being used by FDG, which was already known. Nowhere is it mentioned or can be assumed that it will only be used for that game. The contract provides facilities for anyone to make use of delayed WETH functionality.

Why would any user deposit to DelayedWETH instead of normal WETH?

Maybe because they want to opt in to its security properties?

What can the user do to be labelled as malicious and motivate Optimism to hold the WETH?

Asserting hacks do not exist outside the FDG...

0xlastByte commented 4 months ago

@bizzyvinci can you explain why approve and transfer exists !!! approve function has parameter called guy the same name in hold function i don't think this is a coincidence, and as i mentioned before it says: owner address that can pull WETH from any -> account <- and can recover ETH from the contract itself. as we see here it uses the word account. and as i said before why approve function used for if what you say is true. https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/interfaces/IDelayedWETH.sol#L44

guhu95 commented 4 months ago

Asserting hacks do not exist outside the FDG...

So is the scenario that is now claimed:

Users use DelayedWETH in some other context - unrelated(1) to Optimism's system in scope. They use it contrary to its intended usage (that is well documented).
There is an unrelated(2) exploit in that unrelated(1) system.
When mitigating that exploit using Optimism's admin, there is an edge case that has a workaround(3) that is widely known and used (tx bundle), and only has impact if they wait for nearly 7 days to act(4).

Is that the scenario now?

trust1995 commented 4 months ago

Our design philosophy has been to focus on fundamental safety mechanisms first. We acknowledge that gaining certainty in the correctness of the complex logic found within the FaultDisputeGame contract, its dependencies, and the offchain op-challenger software will take time. Therefore, we have included a number of fallback mechanisms designed to maintain the safety of the system even in the event of a complete failure of those complex components. Our primary goal with this particular contest is to gain confidence in the correctness of these safety mechanisms so that we can safely work on gaining total confidence in the more complex components over time.

As a result, the only focus of this particular contest is on these safety nets and on the points where the fault proving system integrates with existing smart contracts.

Hasn't it already been established that the main thing Optimism were concerned about, is that even if logic goes wrong, their safety assumptions (i.e. the AIRGAPS) work as expected? Is it not clear that this is the part we are clearly breaking in this submission?

The comment above shows no contextual understanding of the contest scope. Will waste no further time arguing with guhu.

guhu95 commented 4 months ago

The comment above shows no contextual understanding of the contest scope. Will waste no further time arguing with guhu.

My comment had no argument, only a request to clarify the updated scenario.

0xlastByte commented 4 months ago

this confirms our statement: https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/interfaces/IDelayedWETH.sol#L44

nevillehuang commented 4 months ago

@0xlastByte @trust1995 Thanks for the additional info, I believe medium is appropriate here, there are code comments here and here that describes the intended use case, and front-running would allow it to potentially bypass the safety mechanism (not taking into account OOS tx bundling services).

Evert0x commented 4 months ago

Yes. Still planning to accept escalation and assign Medium

zobront commented 4 months ago

It feels like this conversation has completely lost the plot. There is no possible argument that DelayedWETH is intended to work outside the FDG context. The contract is completely insecure and accomplishes nothing if used in other contexts (for example, guy is a simple passed value with no permissions, every function is insecure).

It's completely clear that it only serves the purpose of supporting the game. I can't imagine either of the Watsons would be willing to claim they truly believe otherwise.

This "bug" is one of the many ways it wouldn't work outside this context. It doesn't create any risk in this context.

I have trouble imagining how it's being considered.

trust1995 commented 4 months ago

I don't understand the criticism above.

The contract is completely insecure and accomplishes nothing if used in other contexts (for example, guy is a simple passed value with no permissions, every function is insecure).

Really? I actually see all the necessary safety checks, which shouldn't be there if the contract was intended solely for the FDG. guy is a subaccounting method, but the owner of the DelayedWETH balance is in control of their WETH at all times. Functions have overflow checks, balance checks, msg.sender checks, and so forth. If it was purely a drop-in for the FDG, the contract would be a hell of a lot simpler.

We also have several comments as indicated above referring to the user as the "owner" of the token, not the FDG. The scoping rules, which are considered source of truth, do not indicate the DelayedWETH should be considered in the context of the FDG. An airgap breach is demonstrated.

darkbit0 commented 4 months ago

Actually This sentence from above comment is wrong :

The scoping rules, which are considered source of truth, do not indicate the DelayedWETH should be considered in the context of the FDG

These are sentences from contest page:

DelayedWETH includes a mapping that associates attempts to withdraw bonded ETH with specific recipient addresses. However, these specific recipient addresses are not actually required to participate in the process of claiming these withdrawals from the DelayedWETH contract.

This decision was made so that users would not have to interact with the DelayedWETH contract directly and the contract could therefore be removed at a later date without impacting challenger software.

Also this sentence from DelayedWETH comments:

DelayedWETH is designed to be used by the DisputeGame contracts where unlock will only be triggered after a dispute is resolved.

Based on these, It's very clear that the purpose of the DelayedWETH is to secure the process of withdrawing bonds and it's only purpose was to work with FDG and it's gonna be removed later and other are not required to work with DelayedWETH.

It's not mentioned anywhere in contest page that DelayedWETH is a reference implementation and it's gonna be used later for another purposes! It clearly mentions that no one is required to work directly with this contract. This isn't an OZ's library contract, we can't just remove it from the context and define a new purpose for it and find a bug in it. If it was a library we could consider the different usage of the contract and related issues!

According to this from contest page:

Therefore, we have included a number of fallback mechanisms designed to maintain the safety of the system even in the event of a complete failure of those complex components.

The AIRGAP is defined to protect the current system(FDG workflow) and ETH bonds (Protect current system and current context), if you take DelayedWETH out of the context and current system and use it for another purpose, then there's no airgap to bypass or bonded ETH to steal!!! Because airgap and bonded ETH belongs to current context which is defined for this contest.

There is 0 risk for current system and bonded ETHs. Attacker can't perform this attack in current system, so this doesn't compromise current fallback mechanisms of the system, so this doesn't bypass current airgap which was the purpose of this contest.

0xlastByte commented 4 months ago

@darkbit0 as i said why approve is used !!! if it has no purpose it should be disabled but approve exists and it is used and as i mentioned before words like owner, account, were used.

darkbit0 commented 4 months ago

@0xlastByte Why there is weird and extra functionality in DelayedWETH? My guess is: Because of lazy/smart developers. Instead of building something new from zero (which requires more work and introduces more risk) they decided to find most similar secure smart contract for their purpose and change it a little to fit their purpose. Even they mention something about this in the comments:

Variable and function naming vaguely follows the vibe of WETH9. Not the prettiest contract in the world, but it gets the job done.

You can't guess the purpose of a smart contract by function or variables names while there are clear information about this contract in the docs. These are the facts based on contest page and code documentation: (They are explicitly mentioned in Contest README and code comments, more detail in comment)

DelayedWETH is designed to used by DFG.
DelayedWETH is designed to secure bonded ETHs.
DelayedWETH is planned to be removed later (after removing security fallbacks).
Users would not have to interact with the DelayedWETH directly.
Contest purpose was to check fallback mechanisms for current DFG system.

The scenario of this report contradicts all the above facts!!!

zobront commented 4 months ago

Ok, I think we should all pause this argument. It seems like the same points are being made over and over and the judges are capable of understanding without more of the same.

To recap:

1) The context of DelayedWETH seems clear, as well as its role in the contest based on all the comments above. Judges can parse for themselves.

2) More importantly, the issue itself (even if it were in scope) clearly seems to be a Low.

@Evert0x seemed to imply that the "added context" from Trust about the risk changed his mind to accept. But that context totally misrepresents that there is an actual risk.

All it forces the guardian to do is call again. They have off chain monitoring to watch this that is expected to act correctly. The idea that they would be so lost by the approval being revoked that they would screw up for 7 straight days and let it sneak through is a hilarious "contest gaming" argument. C'mon lol.

So basically, we're all saying the same two things:

1) This whole issue is contingent on this being used in another context, which is clearly not how it's intended or a part of this contest.

2) The result of the issue is that the admins need to retry the transaction and batch the calls (either from a contract or with flashbots or any of the other numerous ways). We've all seen 1000 issues that require frontrunning for every transaction indefinitely to work and they are obviously not serious. The only difference here is they need to frontrun every transaction for 7 full days. There's no possible way this happens.

I'm going to leave it at that. Hopefully all the facts are on the table. I think unless there are specific disagreements here, best to just let the judges review all the information and decide. It seems pretty clear cut.

54710adk341 commented 4 months ago

Props to Trust for putting Sherlock in a headlock. Sherlock has totally changed their tune after Trust's tweet about their current state:

https://twitter.com/trust__90/status/1783551207799193965

Sherlock has already lost Optimism as a client, a company they often tout to potential clients. They wouldn't dare risk losing Trust as an LSW. This report will undoubtedly be confirmed, and there's little anyone can do about it—it's simply business.

zobront commented 4 months ago

^ Bad and unfair take.

Judging is hard and unfortunately subjective.

You can tell how I feel about this issue, but it's a massive cope to start blaming things that don't go your way on some conspiracy.

I've seen a lot of Sherlock's decision making over the years and feel very confident in saying that the only thing that goes into these decisions is what they think is most fair.

Don't get me wrong. I think there are many problems with judging (including this issue lol). But I don't believe for a second they aren't trying their best.

54710adk341 commented 4 months ago

Yeah, I agree, it's not a fair assessment, but unfortunately, that's how things are. This report is totally off, but it's going to be accepted anyway. You can't really compare Sherlock's decision-making from a two years ago to now.

If you believe that the departure of key figures like the the best Lead Senior Watsons and Lead Judges is a positive indication of smooth sailing, I'm not sure what else to say.

0xf1b0 commented 4 months ago

I would like to draw attention to one additional point that appears not to have been considered. Alongside the hold() function, there is also an emergency function, recover(), which enables the simple withdrawal of all funds from the contract.

Even if an attacker decides to frontrun every transaction (which, by the way, does not make economic sense), the admin can recover all funds. There is no need to craft private transactions using the mempool.

trust1995 commented 4 months ago

@Evert0x seemed to imply that the "added context" from Trust about the risk changed his mind to accept. But that context totally misrepresents that there is an actual risk.

All it forces the guardian to do is call again. They have off chain monitoring to watch this that is expected to act correctly. The idea that they would be so lost by the approval being revoked that they would screw up for 7 straight days and let it sneak through is a hilarious "contest gaming" argument. C'mon lol.

It's a legitimate airgap-bypass, they don't need to screw up for 7 straight days, that's misrepresentation. They need to not respond in time from the moment they called hold(), which they believe they have 7 days to do. It is a legit flow that they call hold in day 6 and then actually have < 1 day (or < 1 minute) to figure out there is an issue and rush a private mempool TX. If their assumed security model is correct, there is nothing to make that seem unlikely or impossible.

2. More importantly, the issue itself (even if it were in scope) clearly seems to be a Low.

Not when the focus of the context is the security of the safety components and their SLAs.

This whole issue is contingent on this being used in another context, which is clearly not how it's intended or a part of this contest.

We don't agree on that.

It feels like this conversation has completely lost the plot. There is no possible argument that DelayedWETH is intended to work outside the FDG context. The contract is completely insecure and accomplishes nothing if used in other contexts (for example, guy is a simple passed value with no permissions, every function is insecure).

It's completely clear that it only serves the purpose of supporting the game.

This arguments has been completely reversed and the fact there are safety precautions / architecture is generic should be used as evidence not only the FDG integration is in scope. It feels like arguments have been thrown in rapid fire hoping that one will stick.

darkbit0 commented 4 months ago

Just want to remind this from contest page:

The Guardian is assumed to be honest and responsive with an SLA of 24 hours.

So Guardian is trusted to fully resolve issues in 24 hours and they are not gonna wait 7 days to solve issues. So even if attacker prevent them from transferring funds on the first day, Guardian will still have 6 days to handle the issue.

trust1995 commented 4 months ago

The Guardian is assumed responsive, meaning once they have the information of an attack, they will respond in 24 hours. That does not mean the attack will be responded to from the moment it is made! If that was the case, airgap should have been 1 day. Optimism recognizes that attacks take time to identify, triage, confirm, and rightfully give the owner 7 days to respond. When they will, and the frontrun will occur, they will now have a small amount of remaining time, like shown in my previous comment.

Evert0x commented 4 months ago

Within the context of the audit, I believe it's a real issue. To give some general context on the reason why we assign Medium severity to these types of issues.

The frontrunning can be stopped, but intervention from the protocol team (or other actor) is required. Flagging this issue to the protocol team allows them to

a) Fix the issue in the contracts OR b) Setup the necessary off-chain scripts to call this transaction without the chance of frontrunning

Not rewarding this issue will generally make the protocol aware of it when it happens on-chain, and that would be irresponsible. It's not just front-running, it's front-running that can remove an important safety mechanism.

zobront commented 4 months ago

Within the context of the audit, I believe it's a real issue.

@Evert0x I feel the need to respond, because I think you're misunderstanding the issue.

This is not possible in the current setup.

The Fault Dispute Game is the owner of the ETH in the DelayedWETH contract. If you look at DelayedWETH in Optimism's context, no user will hold ANY balance. Only the FDG will hold balance.

This attack is reliant on the holder of the balance called approve(admin, 0). This of course isn't possible when it's the FDG game.

I understand that this is confusing because the guy argument in the withdrawal requests is the user. But the guy in the hold() function is the FDG game. They are unrelated.

I want to confirm this point is clear. This attack is NOT POSSIBLE when DelayedWETH is used in the Optimism context. It is only an issue if it is somehow used in some other context in which a user themselves holds funds. But in this other context, the whole point is moot, because a user who actually holds funds can just call transfer() and take them, there is no withdrawal delay.

Does that all make sense?

0xlastByte commented 4 months ago

@zobront we discussed this in comments above, you are just repeating what we already discussed, you even said the issue is valid and now you are saying its not, you are contradicting yourself.

trust1995 commented 4 months ago

@zobront we both already publicly discussed this multiple times at great length and higher depth than your previous comment, which added 0 new information. This feels like just an attempt to enjoy the bias of having the last word.

zobront commented 4 months ago

I agree it was discussed. I saw @Evert0x write "Within the context of the audit, I believe it's a real issue." and believed he may have missed that context, because (as you point out) that isn't the case.

In that case, do you both agree that ANY instance in which this attack would work would also allow the holder to simply call transfer() to move the funds, and that that is equivalent?

And that this issue cannot impact any user who is being restricted by the timelock, and instead only impacts if the contract that is managing permissions (the holder of the token) is malicious?

trust1995 commented 4 months ago

I would like to draw attention to one additional point that appears not to have been considered. Alongside the hold() function, there is also an emergency function, recover(), which enables the simple withdrawal of all funds from the contract.

Even if an attacker decides to frontrun every transaction (which, by the way, does not make economic sense), the admin can recover all funds. There is no need to craft private transactions using the mempool.

I forgot to respond to this invalid argument. The recover() call is a low-level mechanism with no concept of user's balances. It just grabs ETH from the DelayedWETH contract. This doesn't solve anything unless you want to remove the entire ETH supply in DelayedWETH (which you would need to, otherwise the ETH from another account would be misappropriated during withdraw()). But if you withdraw the entire ETH supply, the entire DelayedWETH is unusable, and all it's dependents, affected or unaffected from the particular attack to be thwarted, are screwed.

In that case, do you both agree that ANY instance in which this attack would work would also allow the holder to simply call transfer() to move the funds, and that that is equivalent?

And that this issue cannot impact any user who is being restricted by the timelock, and instead only impacts if the contract that is managing permissions (the holder of the token) is malicious?

Disagree. We agree that a malicious EOA who is the owner of DelayedWETH can immediately transfer out their tokens, that is a given (and never stated otherwise). The discussion revolves around contracts who are integrating with the DelayedWETH unlock()/withdraw() facility. These would give the false sense of security that would lead users to interact with them, where in essence they may bypass the airgap at any time through the approve() call. That is why Evert is correct that the submission allows Opt to prepare accordingly with either pre-batched transactions, or amend the contract to instantly confiscate user balances.

I feel we've gone overboard with the time allocated to discussions, I suggest we let the judges rule based off all the descriptive knowledge we shared.

zobront commented 4 months ago

Ok, sounds like we're on the same page with the facts. So sounds like the synthesis is that the risky situation is:

A different contract besides FDG interacts with DelayedWETH
Users of that contract are bound by the delayed withdraw mechanism and can't get around it
If the owner of delayed WETH wants to take funds from that contract they would call hold()
The contract itself could upgrade to maliciously call approve() to temporarily block the transfer (my opinion here is that the contract could also upgrade to maliciously call transfer(), not sure if you agree with that)
If the owner of delayed WETH didn't figure out a fix for sufficiently long, it could have bad consequences

Seems like we all agree on the above, so I'm with you that it's just up to judges to rule based on that.

trust1995 commented 4 months ago

My only contention is with this line:

The contract itself could upgrade to maliciously call approve() to temporarily block the transfer (my opinion here is that the contract could also upgrade to maliciously call transfer(), not sure if you agree with that)

An upgradeable contract would always be able to rug users to call transfer(), that would be an OOS centralization risk. The issue is that approve(x, 0) is a very innocent looking line which could easily be viewed as a safety measured by users, but could be abused to bypass the airgap. So a non-upgradeable contract earning trust is likely.

If the owner of delayed WETH didn't figure out a fix for sufficiently long, it could have bad consequences

As pointed earlier, they may have as little as 1 block to come up with a fix, if they assume they have 7 days to hold & confiscate the funds without interruption. So disagree with sufficiently long.

guhu95 commented 4 months ago

Given the above, the scenario is now exclusively in the territory of "an issue within a hypothetical future integration"

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

0xlastByte commented 4 months ago

Given the above, the scenario is now exclusively in the territory of "an issue within a hypothetical future integration"

@trust1995 is answering zobront, you have to understand what is written, you are desperate to make this issue invalid.

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

Trust - M - The safety mechanism of the DelayedWETH contract can be bypassed #193

M - The safety mechanism of the DelayedWETH contract can be bypassed

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation