-
Notifications
You must be signed in to change notification settings - Fork 149
eth: Update v1 contract. #3488
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
base: master
Are you sure you want to change the base?
eth: Update v1 contract. #3488
Conversation
|
Current changes are straight from chatgpt. I took out the part where it "fixed" letting anyone refund. Still looking it over not tested yet. Will put comments back in. Rethinking if maybe we do only want the initiator to refund. |
356f6e0 to
5d5ae86
Compare
|
Has been run through claude a few times. |
4cf9f65 to
730b0f9
Compare
|
Appears to be working. |
| (bool ok,) = payable(recipient).call{value: total - fees}(""); | ||
| require(ok, "AA payout failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have another vulnerability over here. When redeeming using account abstraction, the gas fee is paid to the entrypoint in validateUserOp, but the swap is marked as complete in redeemAA. validateUserOp should only pass if redeemAA will definitely pass, but we have two cases in which validateUserOp can pass but redeemAA will fail, causing some funds to be removed from the contract and sent to the entrypoint without the swap being marked as complete. This can then be run over and over, causing all the funds in the contract to be sent to the entrypoint.
- Making the participant be a smart contract that fails when funds are transfered to it. We can mitigate this by just removing the
require(ok, "AA payout failed");line. - Providing a
callGasLimitin the user op that is too low. We can fix this by checking thecallGasLimitinvalidateUserOp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What keeps validateUserOp from being called by the entrypoint multiple times anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently the protocol only allows it to be called once per user action... ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the last force push. https://github.com/decred/dcrdex/compare/730b0f9ef23df438c823e7b5729a450e56c09b19..78f845ef4f9e948d2149d9b1a88296e41cbaf989
730b0f9 to
78f845e
Compare
|
mmm. Wonder if that's it. 5 fixes by claude● Summary of Fixed Attack Vectors All five attacks exploit the same pattern: validateUserOp succeeds and pays prefund from the contract's pooled
Missing check: validateUserOp verified the signature but not the secret. Attack: Submit UserOp with valid signature but wrong secret.
Fix: Added VALIDATE_INVALID_SECRET check (line 402-404)
Missing check: validateUserOp allowed any token, but redeemAA only supports ETH. Attack: Use an ERC20 token swap for AA redemption.
Fix: Added VALIDATE_TOKEN_NOT_ETH check (line 420)
Missing check: No validation that callGasLimit was sufficient for redeemAA execution. Attack: Submit UserOp with artificially low callGasLimit.
Fix: Added VALIDATE_INSUFFICIENT_CALL_GAS check (lines 431-433)
Missing check: validateUserOp checked blockNum == 0 but not blockNum < block.number. Attack: Initiate swap and submit redemption UserOp in the same block.
Fix: Added blockNum >= block.number to the redeemability check (line 412)
Missing check: validateUserOp didn't require reds.length > 0. Attack: Submit UserOp with empty redemptions array (edge case, requires missingAccountFunds = 0).
Fix: Added VALIDATE_BAD_BATCH_SIZE check (line 393) Root Cause The fundamental issue was that validateUserOp and redeemAA had asymmetric validation logic. Any condition that |
78f845e to
5f102b0
Compare
|
isRedeemed would have returned true after refunded https://github.com/decred/dcrdex/compare/78f845ef4f9e948d2149d9b1a88296e41cbaf989..5f102b003b978664543f47478938ca7a28c14509 |
|
Another interesting attack vector but would have to be the same user attacking themselves so maybe ok? or we can change: self attackC-02 PrepaymentThe ERC-4337 Flow In account abstraction, the EntryPoint processes UserOps in two phases:
How Prepayments Work in This Contract In validateUserOp (lines 416-422): In redeemAA (lines 491-497): The Bug Scenario Two UserOps in the same bundle, both with swap A as their first redemption:
Impact Assessment Who can trigger this?
Exploitability: Low
Severity: Medium (not Critical as ChatGPT claimed)
|
5f102b0 to
bc99194
Compare
|
Should fix the last issue and probably a better scheme than the initial changes. https://github.com/decred/dcrdex/compare/5f102b003b978664543f47478938ca7a28c14509..bc991947035e85e6e4c9946f47002bfb6a0ebd23 |
closes #3487