Fix frontrunning vulnerability with Permit2 signature-based approvals (OZ-01)#48
Fix frontrunning vulnerability with Permit2 signature-based approvals (OZ-01)#48adnanhq wants to merge 5 commits intooz-audit-fixesfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aefbc16a1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.22; | ||
|
|
||
| import {ISignatureTransfer} from "permit2/src/interfaces/ISignatureTransfer.sol"; |
There was a problem hiding this comment.
This new import depends on permit2/src/interfaces/ISignatureTransfer.sol, but the commit does not add a lib/permit2 gitlink to the repository tree (only .gitmodules/remappings are updated), so a clean checkout cannot resolve the dependency. In CI or any fresh clone, forge build will fail to compile these contracts unless Permit2 is installed manually out-of-band, which makes this change release-blocking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e77328d273
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Transfer tokens from the buyer via Permit2. The permit authorises the | ||
| // exact totalAmount and the witness binds all payment parameters. | ||
| IPermit2(INFO.getPermit2Address()).permitWitnessTransferFrom( |
There was a problem hiding this comment.
processCryptoPayment assumes INFO.getPermit2Address() always points to a deployed contract, but this commit hardcodes the canonical address and never checks code.length before calling permitWitnessTransferFrom. On networks where that address has no code (for example local/dev chains or unsupported EVMs), calling this interface method can succeed as a no-op, so execution continues, the payment is recorded, and NFTs can be minted without any token transfer. Add an explicit deployment/code check (or configurable address validation) before relying on Permit2 calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a4f2a7751
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| witnessTypeString = AON_PLEDGE_WITHOUT_REWARD_WITNESS_TYPE_STRING; | ||
| } | ||
|
|
||
| IPermit2(INFO.getPermit2Address()).permitWitnessTransferFrom( |
There was a problem hiding this comment.
_pledge unconditionally calls permitWitnessTransferFrom on INFO.getPermit2Address() without verifying that address has deployed code. On local/dev/unsupported chains where the canonical Permit2 address is empty, this external call can return success as a no-op, so execution continues and the contract mints a pledge NFT and updates raised totals without actually transferring tokens. Please gate this path with a code.length (or equivalent deployment) check before relying on Permit2 transfers.
Useful? React with 👍 / 👎.
| PermitData memory permitData | ||
| ) internal { | ||
| bytes32 internalPledgeId = keccak256(abi.encodePacked(pledgeId, _msgSender())); | ||
| bytes32 internalPledgeId = pledgeId; |
There was a problem hiding this comment.
This change makes s_processedPledges keyed only by raw pledgeId, and the same mapping is shared by public pledge functions and setFeeAndPledge. As a result, a public caller can consume a chosen pledgeId first, causing the later admin webhook path to revert as already processed; this is especially risky when external pledge IDs are predictable or leaked. The previous sender-scoped key avoided this cross-path collision, so the new global key introduces a practical DoS vector against admin processing.
Useful? React with 👍 / 👎.
Reduces contract size across PaymentTreasury variants by removing duplicated refund logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85ac5dcbf6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
adnanhq
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85ac5dcbf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Transfer tokens from the buyer via Permit2. The permit authorises the | ||
| // exact totalAmount and the witness binds all payment parameters. | ||
| IPermit2(INFO.getPermit2Address()).permitWitnessTransferFrom( |
There was a problem hiding this comment.
The new permit flow calls permitWitnessTransferFrom on INFO.getPermit2Address() without verifying that address has code. Because GlobalParams now hardcodes 0x000000000022D473030F116dDEE9F6B43aC78BA3, deploying on a network/fork where Permit2 is not actually deployed will make this external call succeed against an EOA and transfer no tokens, while this function still records the payment and mints an NFT. That creates free purchases and inconsistent treasury accounting. Add a one-time/runtime check that the Permit2 address has contract code before accepting permit-based payments.
Useful? React with 👍 / 👎.
| PermitData memory permitData | ||
| ) internal { | ||
| bytes32 internalPledgeId = keccak256(abi.encodePacked(pledgeId, _msgSender())); | ||
| bytes32 internalPledgeId = pledgeId; |
There was a problem hiding this comment.
Changing internalPledgeId to raw pledgeId makes the processed-pledge key global across all callers/backers. A bot can front-run any pending pledge (or admin setFeeAndPledge) by submitting its own pledge first with the same pledgeId, causing the legitimate transaction to revert with KeepWhatsRaisedPledgeAlreadyProcessed even though the signer/backer is different. This is a new denial-of-service vector introduced by removing caller/signer scoping; key the mapping by pledgeId plus backer (or expected signer) to keep replay protection without cross-user collisions.
Useful? React with 👍 / 👎.
2 participants
Summary
Integrate Uniswap Permit2 to require off-chain EIP-712 signatures for all payment and pledge flows, fixing a critical frontrunning vulnerability where
processCryptoPaymentand_pledgewere callable by anyone with access to a backer's token approvalEach treasury type uses unique witness type hashes (
CryptoPaymentWitness,PledgeForRewardWitness,KWRPledgeForRewardWitness, etc.) to bind application-specific parameters to the signer's signature and prevent cross-contract replayExpose
getPermit2Address()viaGlobalParamsandCampaignInfoso treasuries resolve the canonical Permit2 address on-chainChanges
AllOrNothing.sol/KeepWhatsRaised.sol: Pledge functions now require aPermitDataargument and callpermitWitnessTransferFromto verify the backer's signature before transferring tokensBasePaymentTreasury.sol/PaymentTreasury.sol/TimeConstrainedPaymentTreasury.sol:processCryptoPaymentnow requires aPermitDataargument with a signed witness covering buyer, item ID, and amountGlobalParams.sol/CampaignInfo.sol: AddedgetPermit2Address()returning the canonical Permit2 addressRefactor shared logic of
claimRefundfunctions into_executeRefundinBasePaymentTreasuryto reduce likelihood of hitting contract size limit in PaymentTreasury variants.Update existing tests and include additional signature verification tests.