-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trust - Theft of initial bonds from proposers who are using smart wallets #194
Comments
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 |
What you described is essentially stealing - an honest user's bond will be claimed by the attacker.
|
I think the implied contract is relatively clear, the user who creates the game is the |
So actual stance here is that this is valid but low-likelihood and low-impact in practice. |
The protocol team fixed this issue in the following PRs/commits: |
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 |
@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. |
Escalate The issue is in scope, because:
|
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. |
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. |
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:
Given these clarifications, it would be more accurate to assess the severity of the issue as low, rather than medium. |
@MightyFox3 you seem to have completely missed the meat of the exploit, so allow me to re-iterate:
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.
Of course it requires signatures, this is the part of the original submission:
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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. |
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
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 |
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.
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.
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. |
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. |
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 |
Hey @nevillehuang, Clearly the issue exists in the FaultDisputeGame contract. 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. |
@darkbit0
Was argued above, and neville's response was:
Then:
This was already explored in depth before your attempt to re-open the discussion.
|
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. |
This comment reflects my current stance on this issue #194 (comment) |
Result: |
Escalations have been resolved successfully! Escalation status:
|
??? |
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:
The sender passes a
msg.value
which equals the required bond amount, and the registeredclaimant
istx.origin
. At the end of the game, if the claim is honest, the funds will be returned to theclaimant
.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 theexecute()
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:execTransaction()
or any other variants.msg.value
to the DisputeGameFactory.claimant
will now be the attacker.Impact
Theft of funds from an honest victim who did not interact with the system in any wrong way.
Code Snippet
Tool used
Manual Review
Recommendation
The Factory needs to pass down the real msg.sender to the FaultDisputeGame.
The text was updated successfully, but these errors were encountered: