Fix critical security vulnerabilities: claim verification, secret exposure, input validation#3
Conversation
- Add signature-based claim verification to prevent unauthorized reward draining (claimReward now requires a backend-signed message proving task completion) - Move WalletConnect project ID from hardcoded value to env variable - Add .gitignore to prevent committing secrets, node_modules, build artifacts - Add .env.example files documenting required secrets for contracts and frontend - Add input validation on frontend (tweet URL format, numeric bounds) - Add dotenv to hardhat config for secure secret loading - Add verifier address to constructor and deploy script - Update contract ABI to match new claimReward signature Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add tsconfig.json with @/ path alias for module resolution - Add next.config.js, tailwind.config.ts, postcss.config.js - Handle missing WalletConnect projectId gracefully with fallback + warning (prevents build crash during static generation) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 withdrawFees permanently locks protocolFee if campaign creator never calls withdrawRemaining
The withdrawFees function counts c.protocolFee as locked for all ACTIVE campaigns (line 248). Once a campaign expires, it remains ACTIVE until the creator explicitly calls withdrawRemaining (which sets status to COMPLETED). If the creator loses their key or simply never withdraws, the protocolFee remains permanently locked in the locked calculation. This is pre-existing logic not changed by the PR, but it interacts with the new signature scheme since the verification adds friction that may cause more campaigns to expire with partial participation and uncollected fees.
(Refers to lines 244-249)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const handleClaim = async (campaignId: number) => { | ||
| // In production, fetch the signature from a backend verification service | ||
| // that confirms the user completed the task on Twitter/X | ||
| const response = await fetch(`/api/verify-claim?campaignId=${campaignId}&wallet=${address}`) |
There was a problem hiding this comment.
🟡 handleClaim passes undefined wallet address to API when wallet is disconnected
The handleClaim function (line 91) uses address from useAccount() directly in the fetch URL without checking if the wallet is connected. When isConnected is false, address is undefined, resulting in a fetch to /api/verify-claim?campaignId=1&wallet=undefined. The claim buttons on the browse tab (line 151) have no disabled={!isConnected} guard, so a disconnected user can trigger this. The backend would receive the literal string "undefined" as the wallet parameter, likely producing an invalid signature or error that provides a confusing user experience.
| const handleClaim = async (campaignId: number) => { | |
| // In production, fetch the signature from a backend verification service | |
| // that confirms the user completed the task on Twitter/X | |
| const response = await fetch(`/api/verify-claim?campaignId=${campaignId}&wallet=${address}`) | |
| const handleClaim = async (campaignId: number) => { | |
| if (!isConnected || !address) { | |
| alert('Please connect your wallet first.') | |
| return | |
| } | |
| // In production, fetch the signature from a backend verification service | |
| // that confirms the user completed the task on Twitter/X | |
| const response = await fetch(`/api/verify-claim?campaignId=${campaignId}&wallet=${address}`) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| bytes32 messageHash = keccak256(abi.encodePacked(_campaignId, msg.sender)); | ||
| bytes32 ethSignedHash = messageHash.toEthSignedMessageHash(); | ||
| address signer = ethSignedHash.recover(_signature); | ||
| require(signer == verifier, "Invalid claim signature"); |
There was a problem hiding this comment.
🚩 Signature message does not include contract address or chain ID
The signature hash at contracts/FCFSCampaign.sol:173 is keccak256(abi.encodePacked(_campaignId, msg.sender)). This does not bind the signature to a specific contract deployment or chain. If the same verifier key is used across multiple deployments (e.g., Base Sepolia and Base Mainnet), a signature issued for one deployment's campaign ID N could be replayed on the other deployment's campaign ID N for the same user. Including block.chainid and address(this) in the hash (or using EIP-712 typed data) would prevent cross-chain and cross-contract replay. The hasClaimed mapping prevents same-contract replay, but not cross-deployment replay.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function setVerifier(address _verifier) external onlyOwner { | ||
| require(_verifier != address(0), "Invalid verifier address"); | ||
| verifier = _verifier; | ||
| } |
There was a problem hiding this comment.
🚩 setVerifier invalidates all outstanding signatures without warning
When the owner calls setVerifier to rotate the verifier key, any previously-issued signatures that haven't been submitted on-chain yet become permanently invalid (since recover will return the old address, failing the signer == verifier check at line 176). There's no grace period, no event emitted for off-chain systems to detect the change, and no mechanism to honor both old and new verifiers during a transition period. In production, a verifier rotation during active campaigns could cause users who legitimately completed tasks to lose their claim eligibility.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| const handleClaim = async (campaignId: number) => { | ||
| // In production, fetch the signature from a backend verification service | ||
| // that confirms the user completed the task on Twitter/X | ||
| const response = await fetch(`/api/verify-claim?campaignId=${campaignId}&wallet=${address}`) | ||
| if (!response.ok) { | ||
| alert('Task verification failed. Complete the task first.') | ||
| return | ||
| } | ||
| const { signature } = await response.json() | ||
|
|
||
| writeContract({ | ||
| address: contractAddress, | ||
| abi: contractABI, | ||
| functionName: 'claimReward', | ||
| args: [BigInt(campaignId)], | ||
| args: [BigInt(campaignId), signature as `0x${string}`], | ||
| }) | ||
| } |
There was a problem hiding this comment.
🟡 handleClaim async function lacks try-catch, causing unhandled promise rejections
The handleClaim function is async and used as a click handler (onClick={() => handleClaim(id)} at line 151). If fetch() throws a network error (e.g., offline, DNS failure) or response.json() throws on malformed JSON, the resulting promise rejection is completely unhandled. Since React onClick handlers don't catch async rejections, this leads to an unhandled promise rejection with no user feedback.
Prompt for agents
The handleClaim function in frontend/src/app/page.tsx (lines 88-104) is an async function used as an onClick handler but has no try-catch block. If fetch() throws a network error or response.json() throws on malformed JSON, the error is completely unhandled. Wrap the function body in a try-catch block that catches errors and either shows an alert or sets an error state to inform the user that something went wrong.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -5,15 +5,20 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | |||
| import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | |||
| import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | |||
There was a problem hiding this comment.
🚩 OpenZeppelin v5.x import path for ReentrancyGuard may be incorrect
The contract imports @openzeppelin/contracts/security/ReentrancyGuard.sol at line 6, but the package.json specifies "@openzeppelin/contracts": "^5.0.0". In OpenZeppelin v5.x, ReentrancyGuard was moved to @openzeppelin/contracts/utils/ReentrancyGuard.sol. This would cause a compilation failure. Not reported as a bug per guidelines (compiler would catch it), but flagging for awareness since there are no node_modules or lock files in the repo to verify the actual resolved version.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Addresses multiple security vulnerabilities found during a codebase audit, plus adds missing build configuration that was preventing Vercel deployment.
Critical: Unprotected
claimReward(reward drain vulnerability)Previously anyone could call
claimReward(campaignId)and receive USDC without proving task completion. Now requires ECDSA signature verification from a trusted backend verifier:The frontend now fetches a signature from
/api/verify-claimbefore submitting the claim tx.Critical: Hardcoded WalletConnect Project ID
providers.tsxhadprojectId: 'YOUR_WALLETCONNECT_PROJECT_ID'— moved toprocess.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_IDwith a graceful fallback + console warning.High: No
.gitignore— secrets at risk of being committedAdded comprehensive
.gitignorecovering.env*,node_modules/, build artifacts, and IDE files.Medium: No input validation on campaign creation form
Added URL pattern validation (
x.com|twitter.comstatus links only), numeric bounds checking (reward > 0, participants 1–10,000), and user-facing error messages.Build fix: Missing config files
Added
tsconfig.json(with@/*path alias),next.config.js,tailwind.config.ts,postcss.config.js— these were missing from the repo and blocking both local and Vercel builds.Other
.env.examplefiles for bothcontracts/andfrontend/documenting required secretsdotenvto hardhat config for secure env loadingVERIFIER_ADDRESSenv varpackage-lock.jsonfor reproducible buildsLink to Devin session: https://app.devin.ai/sessions/2739eed836e64215a58bb1be90c003ff
Requested by: @fcfsprojects