fix: Correct voting algorithms and restore anonymous voting security#241
fix: Correct voting algorithms and restore anonymous voting security#241Tech-Psycho95 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Voting Algorithm Fixes: - Moore: fixed majority threshold (totalVotes/2 vs candidatesLength/2) - Quadratic: fixed credit calculation to use N² cost per N votes - IRV: cap iterations, fixed majority calc with active ballots, improved tie handling - Kemeny-Young: simplified to Copeland pairwise method for gas efficiency - Added vote permutation validation to IRV, Ranked, Schulze, KemenyYoung ballots - Added InvalidBallotType/InvalidResultType errors instead of silent fallbacks Election Contract Hardening: - Added election time validation (start < end, start > now) - Fixed removeCandidate to update moved candidate's ID - Added ResultsAlreadyDeclared guard - Fixed electionNotStarted modifier boundary check - Added ballot-result type compatibility validation in factory Anonymous Voting Security Restoration: - Re-enabled authentication routing for unauthenticated users - Fixed admin check to validate wallet against electionOrganizer - Re-enabled identity verification callback in PolygonIDVerifier - Changed socket emit from broadcast to session-specific - Added auth request cleanup timeout to prevent memory leaks - Set allowedIssuers to empty array (reject by default) Client Fixes: - Fixed race condition: switchChain -> await switchChainAsync - Moved Pinata JWT to server-side API route - Fixed GetUserElections to filter by connected user - Fixed SSR compatibility for window.location access
📝 WalkthroughWalkthroughThis pull request introduces comprehensive input validation across blockchain ballot types with permutation checking, enhances server-side socket.io communication with targeted emissions and memory cleanup, implements async chain switching in the client application, adds server-side IPFS proxying, strengthens election time and admin permission checks, and refactors result calculation logic for several voting systems. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
anonymousVoting/clientAnonymousVoting/src/App.js (1)
31-41:⚠️ Potential issue | 🟡 MinorMissing catch-all route for registered users.
When
hasRegisteredis true, there's no catch-all route (path="*"). Unmatched URLs will render nothing instead of redirecting to a known route.🔧 Proposed fix
<Routes> <Route path="/" element={<Navigate to="/auth" />} /> <Route path="/auth" element={<Register/>} /> <Route path="/voting/:id" element={<VotingPage/>} /> <Route path="/createProcess" element={<CreateProcess/>} /> <Route path="/dashboard" element={<Dashboard/>} /> <Route path="/election" element={<Election />} /> + <Route path="*" element={<Navigate to="/dashboard" />} /> </Routes>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@anonymousVoting/clientAnonymousVoting/src/App.js` around lines 31 - 41, The Routes block rendered when hasRegistered is true lacks a catch-all Route, so unmatched URLs render nothing; update the Routes (the JSX that returns <Routes> with <Route path="/" element={<Navigate to="/auth" />} /> and other Routes like "/voting/:id", "/createProcess", "/dashboard", "/election") to include a catch-all Route (path="*") that redirects to a valid route (e.g., Navigate to "/dashboard" or "/auth") so any unknown path is handled and users are redirected appropriately.anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js (1)
53-65:⚠️ Potential issue | 🟠 MajorSocket connection created on every render.
io(serverUrl)is called at module scope (line 53), which means a new socket connection is created on every component render. This can lead to multiple active connections and memory leaks.Move the socket creation inside the
useEffectand include cleanup:🔧 Proposed fix
- const socket = io(serverUrl); - - useEffect(() => { - console.log('url', serverUrl) - socket.on("connect", () => { - setSessionId(socket.id); - - // only watch this session's events - socket.on(socket.id, (arg) => { - setSocketEvents((socketEvents) => [...socketEvents, arg]); - }); - }); - }, []); + useEffect(() => { + console.log('url', serverUrl) + const socket = io(serverUrl); + + socket.on("connect", () => { + setSessionId(socket.id); + + // only watch this session's events + socket.on(socket.id, (arg) => { + setSocketEvents((socketEvents) => [...socketEvents, arg]); + }); + }); + + return () => { + socket.disconnect(); + }; + }, [serverUrl]);Note: This will also require adjusting the
socket.close()call on line 95 to use a ref or state to track the socket instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js` around lines 53 - 65, The socket is being created at module scope via io(serverUrl) causing a new connection on every render; move the io(serverUrl) call into the useEffect that registers listeners, create and assign the socket to a ref (e.g., socketRef.current) or state, register the connect handler there (calling setSessionId and socket.on(socket.id,... ) using socketRef.current), and add a cleanup function that calls socketRef.current.disconnect() or socketRef.current.close() to remove listeners and close the connection; also update the existing socket.close() usage to reference the ref/state instance instead of the module-scoped variable.
🧹 Nitpick comments (5)
client/app/components/Hooks/GetUserElections.ts (1)
26-28: Consider addingenabledguard to skip query when no elections exist.When
allElectionsis undefined or empty,useReadContractsis called with an empty contracts array. While wagmi handles this gracefully, adding anenabledoption makes the intent explicit and avoids unnecessary query lifecycle transitions.♻️ Suggested improvement
- const { data: owners, isLoading: loadingOwners } = useReadContracts({ - contracts: ownerCalls, - }); + const { data: owners, isLoading: loadingOwners } = useReadContracts({ + contracts: ownerCalls, + query: { + enabled: ownerCalls.length > 0, + }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/components/Hooks/GetUserElections.ts` around lines 26 - 28, The hook calls useReadContracts with ownerCalls even when allElections is undefined/empty; add an enabled guard so the query is skipped when there are no elections. Update the useReadContracts call in GetUserElections.ts to pass an options object with enabled set to a boolean expression (e.g., Boolean(allElections?.length) or ownerCalls.length > 0) so the hook runs only when ownerCalls contains contracts, leaving the existing destructuring of data as-is.anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js (1)
92-95: Use numeric timeout value.The
setTimeoutdelay is passed as a string"2000"instead of a number. While JavaScript coerces this, it's clearer to use a numeric literal.✏️ Suggested fix
setTimeout(() => { reportVerificationResult(true); - }, "2000"); + }, 2000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js` around lines 92 - 95, In PolygonIDVerifier update the setTimeout call that currently uses the string "2000" to use a numeric timeout (2000) instead; locate the setTimeout(...) that calls reportVerificationResult(true) and replace the string literal with the number 2000 so the delay is passed as a numeric value, leaving the surrounding logic (reportVerificationResult and socket.close()) unchanged.anonymousVoting/clientAnonymousVoting/src/App.js (1)
34-34: Consider redirecting/to/dashboardfor registered users.Redirecting registered users from
/to/authseems counterintuitive. Typically, authenticated users landing on the root would be sent to a dashboard or home view rather than back to authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@anonymousVoting/clientAnonymousVoting/src/App.js` at line 34, The root Route currently unconditionally navigates to /auth; change it to conditionally redirect based on authentication state by checking the app's auth flag/hook (e.g., isAuthenticated from your auth context or useAuth) and update the Route in App.js so the element uses that flag to Navigate to /dashboard for authenticated users and to /auth for unauthenticated users (or wrap with your existing ProtectedRoute/RequireAuth component if present).client/app/election/[id]/page.tsx (1)
2-4: InitializecurrentUrlfrompathnameand update in a mount effect to prevent hydration mismatch.The server renders
pathname, but the client immediately switches towindow.location.href. SinceClipBoardrendersinputValueon line 16, this divergence causes different markup between server and client on first load. Initialize frompathnameand fill in the full URL within auseEffect:Suggested fix
-import React, { useEffect } from "react"; +import React, { useEffect, useState } from "react"; import { useAccount } from "wagmi"; import { usePathname } from "next/navigation"; @@ -16,7 +16,11 @@ const ElectionPage = ({ params }: { params: { id: `0x${string}` } }) => { const pathname = usePathname(); const electionAddress = params.id; const { electionData, setelectionData } = useElectionData(); + const [currentUrl, setCurrentUrl] = useState(pathname); + useEffect(() => { + setCurrentUrl(window.location.href); + }, [pathname]); - const currentUrl = typeof window !== "undefined" ? window.location.href : `${pathname}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/election/`[id]/page.tsx around lines 2 - 4, The component currently sets currentUrl from window.location.href causing a hydration mismatch; instead initialize currentUrl state from the server-provided pathname (usePathname()) and then in a client-only useEffect update it to the full window.location.href so ClipBoard's inputValue is identical on first render; update the state initialization and add a useEffect that sets currentUrl = window.location.href, referencing the currentUrl state, usePathname(), and the ClipBoard/inputValue usage to locate the code to change.blockchain/contracts/resultCalculators/ResultCalculator.sol (1)
19-41: Good improvement: Explicit revert on invalid result types.Reverting with
InvalidResultType()instead of silently defaulting toGeneralResultis the correct approach for fail-fast behavior and clearer error handling.,
💡 Optional: Consolidate duplicate branches
Result types 1, 2, 5, and 6 all route to
calculateGeneralResult. You could consolidate them:- if (_resultType == 1 || _resultType == 2) { - return calculateGeneralResult(returnData); - } else if (_resultType == 3) { + if (_resultType == 1 || _resultType == 2 || _resultType == 5 || _resultType == 6) { + return calculateGeneralResult(returnData); + } else if (_resultType == 3) { return calculateIRVResult(returnData); } else if (_resultType == 4) { return calculateSchulzeResult(returnData); - } else if (_resultType == 5) { - return calculateGeneralResult(returnData); - } else if (_resultType == 6) { - return calculateGeneralResult(returnData); } else if (_resultType == 7) {However, keeping them separate may be intentional for future differentiation—skip if that's the case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blockchain/contracts/resultCalculators/ResultCalculator.sol` around lines 19 - 41, The getResults function duplicates branches for result types 1, 2, 5, and 6 that all call calculateGeneralResult; simplify by consolidating those branches into a single conditional (e.g., if (_resultType == 1 || _resultType == 2 || _resultType == 5 || _resultType == 6) return calculateGeneralResult(returnData);) while keeping the existing branches for _resultType values 3, 4, 7, 8 and the explicit revert InvalidResultType() unchanged; update getResults to use the consolidated condition to remove redundancy but preserve the same behavior and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@anonymousVoting/clientAnonymousVoting/src/components/Election.js`:
- Around line 116-133: The useEffect's checkAdmin currently only sets
setAdmin(true) and never clears it; modify checkAdmin (used in the Election
component) so that after obtaining address and comparing with
electionDetails.electionOrganizer it explicitly calls setAdmin(false) when the
addresses don't match or when ethereum/electionDetails/electionOrganizer are
missing, and add an ethereum.on('accountsChanged', ...) listener inside the same
useEffect to re-run checkAdmin on account changes (ensure you remove the
listener in the cleanup function); reference the checkAdmin function, setAdmin
state setter, and the useEffect to implement these changes.
In `@anonymousVoting/serverAnonymous/vcHelpers/KYCAgeCredential.js`:
- Around line 8-9: The allowedIssuers array in KYCAgeCredential.js was set to an
empty array which rejects all issuers; update the module so production issuer
DIDs are loaded at runtime by either (a) populating the allowedIssuers constant
with the production issuer DID strings in KYCAgeCredential.js (reference symbol:
allowedIssuers in KYCAgeCredential.js) or (b) add environment variable support
(e.g., ALLOWED_ISSUERS) that the file parses into an array at startup and
assigns to allowedIssuers, validating it is non-empty and logging/throwing a
clear error if not configured; also update
anonymousVoting/serverAnonymous/README.md with instructions for setting allowed
issuers or the ALLOWED_ISSUERS env var and examples.
In `@blockchain/contracts/ballots/IRV.sol`:
- Around line 36-43: The bitmap duplicate-detection in _validatePermutation (and
equivalent validators in KemenyYoungBallot.sol, RankedBallot.sol,
SchulzeBallot.sol) breaks for candidate IDs >= 256 because EVM left-shift wraps
to zero; replace the bitmap approach with a dynamic bool[] seen = new bool[](n)
and then for each arr[i] assert arr[i] < n, if seen[arr[i]] revert
InvalidVotePermutation(), else set seen[arr[i]] = true, or alternatively enforce
a candidate count cap <= 256 at the election layer (Election.sol) if you prefer
keeping the bitmap—apply the same change consistently across the four ballot
contract functions.
In `@blockchain/contracts/ballots/KemenyYoungBallot.sol`:
- Around line 45-52: The permutation-validation bitmap in _validatePermutation
(KemenyYoungBallot.sol) is unsafe for n >= 256 because shifting a uint by >=256
bits invalidates duplicate detection; update the validation to either enforce an
explicit upper bound (e.g., require(n <= 256) with a clear error) or replace the
bitmap logic with a dynamic bool[] seen array and check/set seen[arr[i]] to
detect duplicates; apply the same fix to the other affected validators
(RankedBallot, SchulzeBallot, IRV ballot validation functions) so candidate
counts above 255 are correctly handled.
In `@blockchain/contracts/ballots/RankedBallot.sol`:
- Around line 41-48: The _validatePermutation function uses a single uint bitmap
(bitmap and 1 << arr[i]) which fails for candidate indices >=256; replace the
bitmap approach with a dynamic boolean array: allocate bool[] seen = new
bool[](n), check arr[i] < n, revert on seen[arr[i]] already true, and set
seen[arr[i]] = true to detect duplicates reliably; alternatively, if you intend
to keep the bitmap, enforce an explicit cap by reverting when n > 256 (but
prefer the bool[] seen approach for correctness).
In `@blockchain/contracts/ballots/SchulzeBallot.sol`:
- Around line 41-48: The _validatePermutation function currently uses a 256-bit
bitmap which breaks when totalCandidates > 256 (shifts overflow), so replace the
bitmap approach with a dynamic bool[] memory seen = new bool[](n) and use
seen[arr[i]] checks to detect duplicates; keep the existing bounds check (arr[i]
< n) and revert InvalidVotePermutation on out-of-range or if seen[arr[i]] is
already true, then set seen[arr[i]] = true; this change fixes the overflow
vulnerability in _validatePermutation and accommodates the fact addCandidate
enforces no upper bound on candidate count.
In `@blockchain/contracts/Election.sol`:
- Around line 138-147: The removeCandidate function currently allows the owner
to delete candidates down to zero; update removeCandidate to enforce a minimum
candidate count (e.g., require(candidates.length - 1 >= 2) or check after
removal) so that removing a candidate cannot reduce candidates below the
factory-enforced minimum of 2; add a new custom error declaration (e.g., error
MinimumCandidatesRequired(); ) and use it in removeCandidate (or a require with
a clear message) to revert when removal would violate the minimum; ensure the
check references the candidates array and is applied alongside the existing
swap-and-pop logic in removeCandidate and that the new error symbol is declared
at the top of the contract.
In `@blockchain/contracts/resultCalculators/KemenyYoungResult.sol`:
- Around line 22-25: The implementation of performKemenyYoung currently computes
Copeland winners rather than true Kemeny-Young, so update the contract to avoid
mislabeling: either implement the actual Kemeny-Young algorithm behind the
existing Kemeny-Young API or (preferable) rename/export this implementation as a
distinct Copeland method and add a new result type/value for Copeland;
specifically, create a new function name like performCopeland (or expose
performKemenyYoungAsCopeland) and add/modify the result enum/identifier used by
the contract to include COPLAND (or COPLAND_EQUIVALENT) while leaving
performKemenyYoung unchanged or implementing the correct Kemeny-Young
computation, and update all callers to reference the new function/name (search
for performKemenyYoung, Kemeny-Young result enums, and any consumers of the
result type to update usage).
In `@client/app/api/ipfs/route.ts`:
- Around line 13-26: The handler currently always returns
NextResponse.json(data) which yields HTTP 200 even when the Pinata fetch
response is 4xx/5xx; update the logic around the fetch response (the response
variable from the pinJSONToIPFS request) to check response.ok and only return
the success JSON when true, otherwise return the upstream status and error
payload by calling NextResponse.json with the error body and passing
response.status (and optional statusText) so the client sees the real Pinata
status; adjust the return paths that use response, data, and NextResponse.json
accordingly.
- Around line 39-46: The handler currently accepts any cid and always returns
success; update the logic around const { cid } = await request.json() to
validate that cid is a non-empty string (return a 400 NextResponse when
missing/invalid), then perform the fetch to
https://api.pinata.cloud/pinning/unpin/${cid} and check the upstream response;
only return success when response.ok is true, otherwise return a failure
NextResponse that forwards the upstream status and error body (or a useful error
message). Locate the cid extraction and the fetch call in route.ts to make these
changes.
- Around line 5-51: The /api/ipfs route lacks access control and doesn't
propagate upstream Pinata HTTP errors; add an auth check/middleware to restrict
access (e.g., verify a server-side token/session before invoking handlers) or
validate a request header within the exported POST and DELETE functions, and
keep the existing JWT guard; in both POST and DELETE handlers (functions POST
and DELETE) inspect the fetch response (response.ok / response.status) from
Pinata, parse the response body and return NextResponse.json with that body and
the same status code when non-2xx, and only return success when the upstream
response indicates success—do not always return { success: true } on DELETE and
ensure errors from fetch are logged and returned with a 500 if the request
failed locally.
In `@client/app/election/`[id]/page.tsx:
- Around line 25-34: The page can render stale electionData from the persistent
Zustand store while new electionInformation is still loading; update the sync to
prefer fresh data and clear/store-reset on election id changes: in the component
add electionAddress (or id) and address to the useEffect dependency array and
either 1) set a resolvedElectionData variable that prefers electionInformation
over electionData (e.g. resolvedElectionData = electionInformation ??
electionData) and use that for all downstream destructures, or 2) explicitly
clear the Zustand store (call setelectionData(null) or a reset method) when the
election id/address changes before fetching; ensure useEffect that calls
setelectionData references useElectionInformation/electionInformation,
electionAddress (id) and address so the store is updated deterministically.
In `@client/app/helpers/client.ts`:
- Line 9: The current config sets projectId:
process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID || "" which silently allows an
empty projectId; instead validate NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID at load
time and throw a clear error if it's missing. Update the code that builds the
config (the object containing the projectId key in client/app/helpers/client.ts)
to read the env var into a local const (e.g., const projectId =
process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID) and if (!projectId) throw new
Error("NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID must be set for WalletConnect");
then use that non-null projectId value in the exported config.
In `@client/app/helpers/pinToIPFS.ts`:
- Around line 6-13: The helper currently returns whatever JSON the /api/ipfs
endpoint yields even when the HTTP response is an error; update the pinToIPFS
function to check response.ok after the fetch and, if false, read the response
body (e.g., await response.text() or await response.json()) and throw an Error
(or a custom Error) containing the status and the error body so callers will
receive a thrown exception for 4xx/5xx responses instead of a success payload.
- Around line 22-26: The DELETE call in pinToIPFS.ts currently ignores the fetch
response so unpin failures appear as success; update the function that performs
the fetch (the code block sending method: "DELETE" with body JSON.stringify({
cid: CID })) to await the Response, check response.ok, and throw an Error
(including response.status and/or await response.text() for details) when
!response.ok so callers of pinToIPFS can handle or surface failures.
---
Outside diff comments:
In `@anonymousVoting/clientAnonymousVoting/src/App.js`:
- Around line 31-41: The Routes block rendered when hasRegistered is true lacks
a catch-all Route, so unmatched URLs render nothing; update the Routes (the JSX
that returns <Routes> with <Route path="/" element={<Navigate to="/auth" />} />
and other Routes like "/voting/:id", "/createProcess", "/dashboard",
"/election") to include a catch-all Route (path="*") that redirects to a valid
route (e.g., Navigate to "/dashboard" or "/auth") so any unknown path is handled
and users are redirected appropriately.
In
`@anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js`:
- Around line 53-65: The socket is being created at module scope via
io(serverUrl) causing a new connection on every render; move the io(serverUrl)
call into the useEffect that registers listeners, create and assign the socket
to a ref (e.g., socketRef.current) or state, register the connect handler there
(calling setSessionId and socket.on(socket.id,... ) using socketRef.current),
and add a cleanup function that calls socketRef.current.disconnect() or
socketRef.current.close() to remove listeners and close the connection; also
update the existing socket.close() usage to reference the ref/state instance
instead of the module-scoped variable.
---
Nitpick comments:
In `@anonymousVoting/clientAnonymousVoting/src/App.js`:
- Line 34: The root Route currently unconditionally navigates to /auth; change
it to conditionally redirect based on authentication state by checking the app's
auth flag/hook (e.g., isAuthenticated from your auth context or useAuth) and
update the Route in App.js so the element uses that flag to Navigate to
/dashboard for authenticated users and to /auth for unauthenticated users (or
wrap with your existing ProtectedRoute/RequireAuth component if present).
In
`@anonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.js`:
- Around line 92-95: In PolygonIDVerifier update the setTimeout call that
currently uses the string "2000" to use a numeric timeout (2000) instead; locate
the setTimeout(...) that calls reportVerificationResult(true) and replace the
string literal with the number 2000 so the delay is passed as a numeric value,
leaving the surrounding logic (reportVerificationResult and socket.close())
unchanged.
In `@blockchain/contracts/resultCalculators/ResultCalculator.sol`:
- Around line 19-41: The getResults function duplicates branches for result
types 1, 2, 5, and 6 that all call calculateGeneralResult; simplify by
consolidating those branches into a single conditional (e.g., if (_resultType ==
1 || _resultType == 2 || _resultType == 5 || _resultType == 6) return
calculateGeneralResult(returnData);) while keeping the existing branches for
_resultType values 3, 4, 7, 8 and the explicit revert InvalidResultType()
unchanged; update getResults to use the consolidated condition to remove
redundancy but preserve the same behavior and error handling.
In `@client/app/components/Hooks/GetUserElections.ts`:
- Around line 26-28: The hook calls useReadContracts with ownerCalls even when
allElections is undefined/empty; add an enabled guard so the query is skipped
when there are no elections. Update the useReadContracts call in
GetUserElections.ts to pass an options object with enabled set to a boolean
expression (e.g., Boolean(allElections?.length) or ownerCalls.length > 0) so the
hook runs only when ownerCalls contains contracts, leaving the existing
destructuring of data as-is.
In `@client/app/election/`[id]/page.tsx:
- Around line 2-4: The component currently sets currentUrl from
window.location.href causing a hydration mismatch; instead initialize currentUrl
state from the server-provided pathname (usePathname()) and then in a
client-only useEffect update it to the full window.location.href so ClipBoard's
inputValue is identical on first render; update the state initialization and add
a useEffect that sets currentUrl = window.location.href, referencing the
currentUrl state, usePathname(), and the ClipBoard/inputValue usage to locate
the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1745db4-a80f-4ef9-9611-b99dae784b8a
📒 Files selected for processing (28)
anonymousVoting/clientAnonymousVoting/src/App.jsanonymousVoting/clientAnonymousVoting/src/components/Election.jsanonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.jsanonymousVoting/serverAnonymous/index.jsanonymousVoting/serverAnonymous/vcHelpers/KYCAgeCredential.jsblockchain/contracts/Election.solblockchain/contracts/ElectionFactory.solblockchain/contracts/ballots/BallotGenerator.solblockchain/contracts/ballots/IRV.solblockchain/contracts/ballots/KemenyYoungBallot.solblockchain/contracts/ballots/QuadraticBallot.solblockchain/contracts/ballots/RankedBallot.solblockchain/contracts/ballots/SchulzeBallot.solblockchain/contracts/ballots/interface/Errors.solblockchain/contracts/resultCalculators/IRVResult.solblockchain/contracts/resultCalculators/KemenyYoungResult.solblockchain/contracts/resultCalculators/MooreResult.solblockchain/contracts/resultCalculators/ResultCalculator.solclient/app/api/ipfs/route.tsclient/app/components/Cards/ButtonCard.tsxclient/app/components/Cards/VotingCards/CandidateCard.tsxclient/app/components/Cards/VotingCards/CandidateGrid.tsxclient/app/components/Cards/VotingCards/CreditsVoting.tsxclient/app/components/Helper/CrossChain.tsxclient/app/components/Hooks/GetUserElections.tsclient/app/election/[id]/page.tsxclient/app/helpers/client.tsclient/app/helpers/pinToIPFS.ts
| useEffect(() => { | ||
| if(1) { | ||
| setAdmin(true); | ||
| } | ||
| }, []) | ||
| const checkAdmin = async () => { | ||
| try { | ||
| const { ethereum } = window; | ||
| if (ethereum && electionDetails && electionDetails.electionOrganizer) { | ||
| const provider = new ethers.providers.Web3Provider(ethereum); | ||
| const signer = provider.getSigner(); | ||
| const address = await signer.getAddress(); | ||
| if (address.toLowerCase() === electionDetails.electionOrganizer.toLowerCase()) { | ||
| setAdmin(true); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.log(err); | ||
| } | ||
| }; | ||
| checkAdmin(); | ||
| }, [electionDetails]) |
There was a problem hiding this comment.
Admin state never resets to false.
The checkAdmin effect only sets setAdmin(true) when addresses match but never resets to false when they don't. If the user switches wallets or disconnects, isAdmin remains true from a previous check.
🔧 Proposed fix
useEffect(() => {
const checkAdmin = async () => {
try {
const { ethereum } = window;
if (ethereum && electionDetails && electionDetails.electionOrganizer) {
const provider = new ethers.providers.Web3Provider(ethereum);
const signer = provider.getSigner();
const address = await signer.getAddress();
if (address.toLowerCase() === electionDetails.electionOrganizer.toLowerCase()) {
setAdmin(true);
- }
+ } else {
+ setAdmin(false);
+ }
+ } else {
+ setAdmin(false);
}
} catch (err) {
console.log(err);
+ setAdmin(false);
}
};
checkAdmin();
}, [electionDetails])Additionally, consider listening for account changes via ethereum.on('accountsChanged', ...) to re-check admin status when the user switches wallets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anonymousVoting/clientAnonymousVoting/src/components/Election.js` around
lines 116 - 133, The useEffect's checkAdmin currently only sets setAdmin(true)
and never clears it; modify checkAdmin (used in the Election component) so that
after obtaining address and comparing with electionDetails.electionOrganizer it
explicitly calls setAdmin(false) when the addresses don't match or when
ethereum/electionDetails/electionOrganizer are missing, and add an
ethereum.on('accountsChanged', ...) listener inside the same useEffect to re-run
checkAdmin on account changes (ensure you remove the listener in the cleanup
function); reference the checkAdmin function, setAdmin state setter, and the
useEffect to implement these changes.
| // TODO: Configure production issuers. An empty array rejects all issuers by default. | ||
| allowedIssuers: [], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or environment variable handling for allowed issuers
rg -n "allowedIssuers" --type js
rg -n "ALLOWED_ISSUERS" --type-add 'env:*.env*' -t envRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 160
🏁 Script executed:
cat -n anonymousVoting/serverAnonymous/vcHelpers/KYCAgeCredential.jsRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 918
🏁 Script executed:
find . -type f \( -name "*deploy*" -o -name "*config*" -o -name "README*" \) | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 588
🏁 Script executed:
rg -n "allowedIssuers" -B 5 -A 5 --type jsRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 1197
🏁 Script executed:
cat anonymousVoting/serverAnonymous/README.mdRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 7440
🏁 Script executed:
rg -n "issuer" --type mdRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 419
🏁 Script executed:
# Check how this credential helper is used in the codebase
rg -n "KYCAgeCredential" -B 2 -A 2 --type jsRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 2618
Empty allowedIssuers must be configured before deployment.
Changing from ["*"] to [] correctly removes the insecure wildcard issuer, but an empty array rejects all credentials. The deployment documentation (anonymousVoting/serverAnonymous/README.md) does not include steps to configure allowed issuers. Add configuration instructions covering either:
- How to update
allowedIssuersarray with production issuer DIDs - Environment variable support (e.g.,
ALLOWED_ISSUERS) to inject issuer configuration at runtime
Without this, the verification will fail for all users in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@anonymousVoting/serverAnonymous/vcHelpers/KYCAgeCredential.js` around lines 8
- 9, The allowedIssuers array in KYCAgeCredential.js was set to an empty array
which rejects all issuers; update the module so production issuer DIDs are
loaded at runtime by either (a) populating the allowedIssuers constant with the
production issuer DID strings in KYCAgeCredential.js (reference symbol:
allowedIssuers in KYCAgeCredential.js) or (b) add environment variable support
(e.g., ALLOWED_ISSUERS) that the file parses into an array at startup and
assigns to allowedIssuers, validating it is non-empty and logging/throwing a
clear error if not configured; also update
anonymousVoting/serverAnonymous/README.md with instructions for setting allowed
issuers or the ALLOWED_ISSUERS env var and examples.
| function _validatePermutation(uint[] memory arr, uint n) internal pure { | ||
| uint bitmap = 0; | ||
| for (uint i = 0; i < n; i++) { | ||
| if (arr[i] >= n) revert InvalidVotePermutation(); | ||
| uint bit = 1 << arr[i]; | ||
| if (bitmap & bit != 0) revert InvalidVotePermutation(); | ||
| bitmap |= bit; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "IRV.sol" -type fRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 109
🏁 Script executed:
find . -name "*Election*.sol" -type f | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 149
🏁 Script executed:
find . -path "*/ballots/*.sol" -type fRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 562
🏁 Script executed:
cat -n ./blockchain/contracts/ballots/IRV.sol | head -100Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 1656
🏁 Script executed:
wc -l ./blockchain/contracts/ballots/IRV.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 112
🏁 Script executed:
cat -n ./blockchain/contracts/Election.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 6473
🏁 Script executed:
wc -l ./blockchain/contracts/Election.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 110
🏁 Script executed:
grep -r "_validatePermutation" ./blockchain/contracts/ballots/ --include="*.sol"Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 980
🏁 Script executed:
grep -r "bitmap" ./blockchain/contracts/ballots/ --include="*.sol" -nRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 1153
🏁 Script executed:
cat -n ./blockchain/contracts/ElectionFactory.sol | head -150Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 6630
🏁 Script executed:
find ./blockchain/test -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "candidate\|bitmap\|validatePermutation" 2>/dev/null | head -10Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 514
🏁 Script executed:
find ./blockchain/test -name "*ballot*" -o -name "*Ballot*" | head -10Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 53
🏁 Script executed:
grep -n "candidate\|bitmap\|validatePermutation\|256" ./blockchain/test/unit/IRVVoting.test.js | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 675
🏁 Script executed:
cat -n ./blockchain/test/unit/IRVVoting.test.js | head -80Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 3602
🏁 Script executed:
grep -n "256\|bit\|bitmap" ./blockchain/contracts/ballots/*.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 2497
🌐 Web query:
Solidity uint256 shift operator 1 << 256 behavior overflow wrap
💡 Result:
In Solidity on the EVM, uint256(1) << 256 evaluates to 0 (it does not wrap to 1, and it does not revert).
Why:
- The EVM
SHLopcode specifies: if the shift amount is ≥ 256, the result is 0. So shifting left by exactly 256 clears the value. [1] - Solidity shift operations never perform overflow checks; instead the result is always truncated to the left operand’s type (here
uint256). So there’s no “checked overflow revert” behavior for<<. [2]
Implication: you cannot represent 2**256 in a uint256; 1 << 255 is the largest single-bit value representable, and 1 << 256 becomes 0. [1][2]
Sources: [1] EIP-145 (EVM SHL) https://eips.ethereum.org/EIPS/eip-145, [2] Solidity documentation (Shifts: truncation; no overflow checks) https://docs.soliditylang.org/ (PDF download)
Bitmap-based duplicate detection fails for candidate IDs ≥ 256.
In Solidity, 1 << arr[i] returns 0 when arr[i] >= 256 (EVM SHL opcode specification). This causes the duplicate check to silently pass for invalid permutations with IDs ≥ 256. Since Election.sol enforces no upper candidate limit, any caller can create elections with > 256 candidates and bypass validation.
This issue affects 4 ballot contracts: IRV.sol, KemenyYoungBallot.sol, RankedBallot.sol, and SchulzeBallot.sol. Either add an explicit candidate count cap (≤ 256) in the election layer or replace the bitmap with a bool[] seen array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blockchain/contracts/ballots/IRV.sol` around lines 36 - 43, The bitmap
duplicate-detection in _validatePermutation (and equivalent validators in
KemenyYoungBallot.sol, RankedBallot.sol, SchulzeBallot.sol) breaks for candidate
IDs >= 256 because EVM left-shift wraps to zero; replace the bitmap approach
with a dynamic bool[] seen = new bool[](n) and then for each arr[i] assert
arr[i] < n, if seen[arr[i]] revert InvalidVotePermutation(), else set
seen[arr[i]] = true, or alternatively enforce a candidate count cap <= 256 at
the election layer (Election.sol) if you prefer keeping the bitmap—apply the
same change consistently across the four ballot contract functions.
| function _validatePermutation(uint[] memory arr, uint n) internal pure { | ||
| uint bitmap = 0; | ||
| for (uint i = 0; i < n; i++) { | ||
| if (arr[i] >= n) revert InvalidVotePermutation(); | ||
| uint bit = 1 << arr[i]; | ||
| if (bitmap & bit != 0) revert InvalidVotePermutation(); | ||
| bitmap |= bit; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine KemenyYoungBallot.sol
fd KemenyYoungBallot.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 121
🏁 Script executed:
# Search for candidate count constraints and initialization
rg "candidateCount|numCandidates|candidate.*limit|MAX.*CANDIDATE" blockchain/contracts --type sol -A 3 -B 3Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 98
🏁 Script executed:
# Look for _validatePermutation usage and calls
rg "_validatePermutation" blockchain/ -A 5 -B 5Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 7349
🏁 Script executed:
# Check election layer for candidate count validation
rg "createElection|initializeElection|initialize" blockchain/contracts --type sol -A 10 -B 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 98
🏁 Script executed:
# Read KemenyYoungBallot.sol to see initialization and full context
cat blockchain/contracts/ballots/KemenyYoungBallot.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 1718
🏁 Script executed:
# Search for ballot initialization/constructor patterns
grep -r "constructor\|initialize" blockchain/contracts/ballots/ --include="*.sol" -A 10Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 6322
🏁 Script executed:
# Look for candidate count constraints or constants
grep -r "MAX\|LIMIT\|256\|candidates" blockchain/contracts --include="*.sol" -B 2 -A 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 31688
🏁 Script executed:
# Check test files for permutation validation tests
find blockchain/test -name "*.test.js" -o -name "*.spec.js" | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 534
🏁 Script executed:
# Check test files for KemenyYoung to see what candidate counts are tested
cat blockchain/test/unit/KementYoungVoting.test.js | head -100Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 3712
🏁 Script executed:
# Search for any max candidate constraints or limits in tests
rg "candidate|total.*candidate" blockchain/test/ -B 2 -A 2 | head -100Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 7268
🏁 Script executed:
# Verify EVM shift behavior: what happens with 1 << 256 in Solidity
cat > /tmp/test_shift.sol << 'EOF'
pragma solidity ^0.8.24;
contract ShiftTest {
function testShift() public pure returns (uint) {
uint bit256 = 1 << 256; // Should be 0
return bit256;
}
function testShift255() public pure returns (uint) {
uint bit255 = 1 << 255; // Should be valid
return bit255;
}
}
EOF
cat /tmp/test_shift.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 393
🏁 Script executed:
# Check ElectionFactory for candidate count validation/limits
cat blockchain/contracts/ElectionFactory.sol | grep -A 20 "createElection"Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 929
🏁 Script executed:
# Look for any documentation or comments about candidate count limits
rg "256|limit|max|candidate.*cap" blockchain/contracts --type sol -B 2 -A 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 98
🏁 Script executed:
# Check if there are tests that attempt to use large candidate counts
rg "candidate.*[0-9]{3,}" blockchain/test --include="*.js" -B 2 -A 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 512
🏁 Script executed:
# Search for large candidate counts in tests without type filter
grep -r "candidate" blockchain/test --include="*.js" | grep -E "[0-9]{2,}" | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 373
🏁 Script executed:
# Check ElectionFactory.sol fully for candidate limits
cat blockchain/contracts/ElectionFactory.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 5935
🏁 Script executed:
# Look at what Solidity version comment says about shifts >= 256
grep -r "shift\|<<\|>>" blockchain/contracts --include="*.sol" -B 3 -A 3Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 2414
🏁 Script executed:
# Check if there's any documentation about candidate limits in README or similar
find blockchain -name "README*" -o -name "*.md" | xargs cat 2>/dev/nullRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 1113
_validatePermutation bitmap only works reliably for candidate counts up to 255.
In Solidity, shifting a uint256 by >= 256 bits wraps around or produces zero. If an election were created with 256+ candidates, the duplicate-detection bitmap would fail—candidate IDs >= 256 would not map to unique bits, allowing invalid permutations to pass validation. Since ElectionFactory.sol imposes no candidate count limit (only checking minimum ≥ 2), this creates an unprotected edge case. Add an explicit cap on candidate count (e.g., require n <= 256 in _validatePermutation or at election initialization), or replace the bitmap with a bool[] seen array.
This pattern appears in KemenyYoungBallot, RankedBallot, SchulzeBallot, and IRV ballots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blockchain/contracts/ballots/KemenyYoungBallot.sol` around lines 45 - 52, The
permutation-validation bitmap in _validatePermutation (KemenyYoungBallot.sol) is
unsafe for n >= 256 because shifting a uint by >=256 bits invalidates duplicate
detection; update the validation to either enforce an explicit upper bound
(e.g., require(n <= 256) with a clear error) or replace the bitmap logic with a
dynamic bool[] seen array and check/set seen[arr[i]] to detect duplicates; apply
the same fix to the other affected validators (RankedBallot, SchulzeBallot, IRV
ballot validation functions) so candidate counts above 255 are correctly
handled.
| function _validatePermutation(uint[] memory arr, uint n) internal pure { | ||
| uint bitmap = 0; | ||
| for (uint i = 0; i < n; i++) { | ||
| if (arr[i] >= n) revert InvalidVotePermutation(); | ||
| uint bit = 1 << arr[i]; | ||
| if (bitmap & bit != 0) revert InvalidVotePermutation(); | ||
| bitmap |= bit; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the RankedBallot.sol file
find . -name "RankedBallot.sol" -type fRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 118
🏁 Script executed:
# Check the actual implementation of _validatePermutation and surrounding context
cat -n blockchain/contracts/ballots/RankedBallot.sol | head -80Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 1975
🏁 Script executed:
# Search for where _validatePermutation is called to understand what values of n are used
rg "_validatePermutation" --type solidity -B 2 -A 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 103
🏁 Script executed:
# Search for any constraints on candidate count in the election layer
rg "candidates|candidateCount|maxCandidates" blockchain/contracts --type solidity -A 1 -B 1Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 103
🏁 Script executed:
# Check if there are any tests that verify the candidate count limits
rg "_validatePermutation|candidate.*256|bitmap" blockchain/test --type javascriptRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 105
🏁 Script executed:
# Search for where RankedBallot.init is called (Election contract)
rg "init\(" blockchain/contracts --type-list | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 460
🏁 Script executed:
# Try again without type specification
rg "\.init\(" blockchain/contracts -A 2 -B 2Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 791
🏁 Script executed:
# Search for Election contract that uses RankedBallot
rg "RankedBallot" blockchain/contractsRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 362
🏁 Script executed:
# Look for any candidate count or limit constants
rg "Candidate|candidate|MAX_" blockchain/contracts/*.solRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 2354
🏁 Script executed:
# Check test files for how init is called (use pattern matching)
find blockchain/test -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "init\|RankedBallot" 2>/dev/null | head -5Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 275
🏁 Script executed:
# Check the actual Election.sol file to understand the candidates array and any limits
cat -n blockchain/contracts/Election.sol | grep -A 20 "Candidate\[\]"Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 1906
🏁 Script executed:
# Look at test files for typical candidate counts
grep -r "candidates\|totalCandidate" blockchain/test/*.js | grep -E "length|count|= [0-9]" | head -20Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 53
🏁 Script executed:
# Check if there's documentation or constraints in comments
grep -B 5 -A 5 "candidates" blockchain/contracts/Election.sol | head -40Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 1315
🏁 Script executed:
# Look at specific test for RankedBallot voting
find blockchain/test -name "*.js" -exec grep -l "RankedBallot\|_validatePermutation" {} \;Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 53
🏁 Script executed:
# Search for test initialization patterns with specific candidate counts
rg "new Election\|createElection\|candidates.*length|totalCandidate" blockchain/test --context 3Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 53
🏁 Script executed:
# Look at IRVVoting test which likely tests RankedBallot
cat blockchain/test/unit/IRVVoting.test.js | head -150Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 5882
🏁 Script executed:
# Check lifecycle tests for candidate setup
find blockchain/test/lifecycle -name "*.test.js" -exec head -100 {} \;Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 131
_validatePermutation is only correct up to 256 candidates.
This bitmap-based duplicate check relies on a single uint. In Solidity 0.8.24, the shift operator 1 << arr[i] wraps when arr[i] >= 256, causing the duplicate detection to fail silently. Since the election layer does not enforce n <= 256, invalid permutations with candidate IDs >= 256 can be accepted. Either add an explicit cap at the election layer or replace the bitmap with a bool[] seen array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blockchain/contracts/ballots/RankedBallot.sol` around lines 41 - 48, The
_validatePermutation function uses a single uint bitmap (bitmap and 1 << arr[i])
which fails for candidate indices >=256; replace the bitmap approach with a
dynamic boolean array: allocate bool[] seen = new bool[](n), check arr[i] < n,
revert on seen[arr[i]] already true, and set seen[arr[i]] = true to detect
duplicates reliably; alternatively, if you intend to keep the bitmap, enforce an
explicit cap by reverting when n > 256 (but prefer the bool[] seen approach for
correctness).
| const { cid } = await request.json(); | ||
|
|
||
| await fetch(`https://api.pinata.cloud/pinning/unpin/${cid}`, { | ||
| method: 'DELETE', | ||
| headers: { Authorization: `Bearer ${JWT}` }, | ||
| }); | ||
|
|
||
| return NextResponse.json({ success: true }); |
There was a problem hiding this comment.
Don’t acknowledge unpins that Pinata rejected.
This handler never validates cid and ignores the upstream status, so /unpin/undefined or any Pinata 4xx/5xx still returns { success: true }. Reject empty CIDs and only return success when response.ok is true.
Suggested fix
try {
const { cid } = await request.json();
+ if (!cid || typeof cid !== 'string') {
+ return NextResponse.json({ error: 'cid is required' }, { status: 400 });
+ }
- await fetch(`https://api.pinata.cloud/pinning/unpin/${cid}`, {
+ const response = await fetch(`https://api.pinata.cloud/pinning/unpin/${cid}`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${JWT}` },
});
- return NextResponse.json({ success: true });
+ if (!response.ok) {
+ return NextResponse.json({ error: 'Failed to unpin from IPFS' }, { status: response.status });
+ }
+
+ return NextResponse.json({ success: true });
} catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/api/ipfs/route.ts` around lines 39 - 46, The handler currently
accepts any cid and always returns success; update the logic around const { cid
} = await request.json() to validate that cid is a non-empty string (return a
400 NextResponse when missing/invalid), then perform the fetch to
https://api.pinata.cloud/pinning/unpin/${cid} and check the upstream response;
only return success when response.ok is true, otherwise return a failure
NextResponse that forwards the upstream status and error body (or a useful error
message). Locate the cid extraction and the fetch call in route.ts to make these
changes.
| // Use useEffect to update state to avoid setting state during render | ||
| useEffect(() => { | ||
| if (electionInformation) { | ||
| setelectionData(electionInformation); | ||
| } | ||
| }, [electionInformation, setelectionData]); | ||
|
|
||
| if (electionData !== electionInformation) { | ||
| setelectionData(electionInformation); | ||
| } | ||
| if (isLoading) return <Loader />; | ||
|
|
||
| if (!electionData) return <Loader />; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether `useElectionData` is shared/persistent and whether
# `useElectionInformation` returns fresh data independently of that store.
fd 'ElectionInfo.*' client/app/hooks -x sh -c 'echo "== $1 =="; sed -n "1,220p" "$1"' sh {}
fd 'GetElectionInformation.*' client/app/components/Hooks -x sh -c 'echo "== $1 =="; sed -n "1,220p" "$1"' sh {}Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 2131
🏁 Script executed:
cat -n client/app/election/[id]/page.tsx | head -100Repository: AOSSIE-Org/Agora-Blockchain
Length of output: 4370
Avoid rendering stale election data when navigating between elections.
When switching to a different election, useElectionInformation fetches fresh data asynchronously while useElectionData (a persistent Zustand store) still holds the previous election's information. If isLoading becomes false before the useEffect syncs the new data, the page renders with the wrong owner/candidates/actions. Additionally, the effect's dependency array (line 30) omits electionAddress and address, making the synchronization implicit rather than explicit.
Prefer rendering from fresh data by checking electionInformation first, or clear the store when the election ID changes. The suggested refactor below addresses both timing and clarity:
Suggested refactor
useEffect(() => {
if (electionInformation) {
setelectionData(electionInformation);
}
- }, [electionInformation, setelectionData]);
+ }, [electionInformation, setelectionData, electionAddress, address]);
- if (isLoading) return <Loader />;
+ const resolvedElectionData = electionInformation ?? electionData;
+
+ if (isLoading || !resolvedElectionData) return <Loader />;Then update all electionData[n] destructures (lines 35–44) to use resolvedElectionData[n].
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use useEffect to update state to avoid setting state during render | |
| useEffect(() => { | |
| if (electionInformation) { | |
| setelectionData(electionInformation); | |
| } | |
| }, [electionInformation, setelectionData]); | |
| if (electionData !== electionInformation) { | |
| setelectionData(electionInformation); | |
| } | |
| if (isLoading) return <Loader />; | |
| if (!electionData) return <Loader />; | |
| // Use useEffect to update state to avoid setting state during render | |
| useEffect(() => { | |
| if (electionInformation) { | |
| setelectionData(electionInformation); | |
| } | |
| }, [electionInformation, setelectionData, electionAddress, address]); | |
| const resolvedElectionData = electionInformation ?? electionData; | |
| if (isLoading || !resolvedElectionData) return <Loader />; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/election/`[id]/page.tsx around lines 25 - 34, The page can render
stale electionData from the persistent Zustand store while new
electionInformation is still loading; update the sync to prefer fresh data and
clear/store-reset on election id changes: in the component add electionAddress
(or id) and address to the useEffect dependency array and either 1) set a
resolvedElectionData variable that prefers electionInformation over electionData
(e.g. resolvedElectionData = electionInformation ?? electionData) and use that
for all downstream destructures, or 2) explicitly clear the Zustand store (call
setelectionData(null) or a reset method) when the election id/address changes
before fetching; ensure useEffect that calls setelectionData references
useElectionInformation/electionInformation, electionAddress (id) and address so
the store is updated deterministically.
| export const config = getDefaultConfig({ | ||
| appName: "Agora-Blockchain", | ||
| projectId: "8501447cf73c4e68061f7ed912d6a8ee", | ||
| projectId: process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID || "", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For @rainbow-me/rainbowkit2.1.2 / WalletConnect integration, isprojectId required to be non-empty during initialization, and do the official docs recommend failing fast when the environment variable is missing?
💡 Result:
Yes—if you’re using WalletConnect via RainbowKit (including @rainbow-me/rainbowkit@2.1.2), projectId is expected to be provided (i.e., a real, non-empty WalletConnect/Reown Cloud Project ID). RainbowKit’s configuration APIs and examples treat projectId as a required input (they show it as a mandatory field and commonly wire it from an env var with a non-null assertion). [2]
On “fail fast”: RainbowKit’s own docs/examples don’t explicitly say “throw if the env var is missing”; they mostly rely on TypeScript patterns like process.env.X! / “YOUR_PROJECT_ID” placeholders rather than a runtime guard. [2] However, WalletConnect/Reown’s official React installation docs do explicitly recommend a runtime check and show throwing an error when projectId is missing (if (!projectId) { throw new Error('Project ID is not defined') }). [1]
Sources: [1] WalletConnect/Reown App SDK React installation docs, “Cloud Configuration” example with explicit throw guard; [2] RainbowKit configuration docs/examples showing projectId as required input (and using process.env...!).
Fail fast when NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID is missing.
Line 9 falls back to "" when the env var is absent. projectId must be non-empty for WalletConnect initialization, and the official WalletConnect/Reown setup guidance explicitly recommends throwing an error when it is missing. The empty string fallback masks a deployment misconfiguration and causes failures later at runtime.
Suggested change
+const walletConnectProjectId =
+ process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID;
+
+if (!walletConnectProjectId) {
+ throw new Error("Missing NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID");
+}
+
export const config = getDefaultConfig({
appName: "Agora-Blockchain",
- projectId: process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID || "",
+ projectId: walletConnectProjectId,
chains: [sepolia, avalancheFuji],
ssr: true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projectId: process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID || "", | |
| const walletConnectProjectId = | |
| process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID; | |
| if (!walletConnectProjectId) { | |
| throw new Error("Missing NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID"); | |
| } | |
| export const config = getDefaultConfig({ | |
| appName: "Agora-Blockchain", | |
| projectId: walletConnectProjectId, | |
| chains: [sepolia, avalancheFuji], | |
| ssr: true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/helpers/client.ts` at line 9, The current config sets projectId:
process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID || "" which silently allows an
empty projectId; instead validate NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID at load
time and throw a clear error if it's missing. Update the code that builds the
config (the object containing the projectId key in client/app/helpers/client.ts)
to read the env var into a local const (e.g., const projectId =
process.env.NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID) and if (!projectId) throw new
Error("NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID must be set for WalletConnect");
then use that non-null projectId value in the exported config.
| const response = await fetch("/api/ipfs", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| console.log(data); | ||
| return data; |
There was a problem hiding this comment.
Throw when the pin proxy responds with an error.
fetch only rejects on transport failures. A 4xx/5xx from /api/ipfs still resolves here, so callers can continue with an error payload as if pinning succeeded.
Suggested fix
try {
const response = await fetch("/api/ipfs", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(body),
});
const data = await response.json();
+ if (!response.ok) {
+ throw new Error(data?.error ?? "Failed to pin JSON to IPFS");
+ }
return data;
} catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/helpers/pinToIPFS.ts` around lines 6 - 13, The helper currently
returns whatever JSON the /api/ipfs endpoint yields even when the HTTP response
is an error; update the pinToIPFS function to check response.ok after the fetch
and, if false, read the response body (e.g., await response.text() or await
response.json()) and throw an Error (or a custom Error) containing the status
and the error body so callers will receive a thrown exception for 4xx/5xx
responses instead of a success payload.
| await fetch("/api/ipfs", { | ||
| method: "DELETE", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ cid: CID }), | ||
| }); |
There was a problem hiding this comment.
Propagate unpin failures to callers.
This helper ignores the HTTP status completely, so failed deletes resolve as success. Read the response and throw on !response.ok so the caller can recover or notify the user.
Suggested fix
try {
- await fetch("/api/ipfs", {
+ const response = await fetch("/api/ipfs", {
method: "DELETE",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ cid: CID }),
});
+
+ if (!response.ok) {
+ const data = await response.json().catch(() => null);
+ throw new Error(data?.error ?? "Failed to unpin JSON from IPFS");
+ }
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await fetch("/api/ipfs", { | |
| method: "DELETE", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ cid: CID }), | |
| }); | |
| try { | |
| const response = await fetch("/api/ipfs", { | |
| method: "DELETE", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ cid: CID }), | |
| }); | |
| if (!response.ok) { | |
| const data = await response.json().catch(() => null); | |
| throw new Error(data?.error ?? "Failed to unpin JSON from IPFS"); | |
| } | |
| } catch (err) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/helpers/pinToIPFS.ts` around lines 22 - 26, The DELETE call in
pinToIPFS.ts currently ignores the fetch response so unpin failures appear as
success; update the function that performs the fetch (the code block sending
method: "DELETE" with body JSON.stringify({ cid: CID })) to await the Response,
check response.ok, and throw an Error (including response.status and/or await
response.text() for details) when !response.ok so callers of pinToIPFS can
handle or surface failures.
|
good work fixing the QuadraticBallot and RankedBallot logic
not blocking the PR just flagging for completeness. happy to open PRs for any of these. |
2 participants
Summary
This PR addresses critical flaws in voting algorithms and restores security safeguards in the anonymous voting module. These fixes ensure election results are mathematically correct before being permanently stored on-chain, and restore the identity verification and security mechanisms that were previously bypassed.
🗳️ Voting Algorithm Fixes
Moore's Method (
MooreResult.sol)Bug: Majority threshold incorrectly used
candidatesLength / 2instead oftotalVotes / 2Fix: Now correctly calculates total votes and checks if any candidate exceeds 50% of actual votes cast.
Quadratic Voting (
QuadraticBallot.sol)Bug: Credit calculation was linear (subtracting votes) instead of quadratic (N votes should cost N² credits)
Fix: Properly implements quadratic cost calculation where allocating N votes to a candidate costs N² credits out of 100 total.
IRV - Instant Runoff Voting (
IRVResult.sol,IRV.sol)Bugs:
while(true)loop could cause DoSFixes:
numCandidatesto prevent unbounded loops_validatePermutation()to ensure valid rankingsKemeny-Young (
KemenyYoungResult.sol,KemenyYoungBallot.sol)Bug: Complex O(n²) pairwise scoring with incorrect winner determination
Fix: Simplified to Copeland pairwise comparison method (gas-efficient approximation). Each candidate scores +1 for each pairwise contest they win.
Vote Permutation Validation (Multiple Ballots)
Bug: Ranked voting ballots (IRV, Ranked, Schulze, KemenyYoung) did not validate that vote arrays were valid permutations
Fix: Added
_validatePermutation()using bitmap to ensure:Added to:
IRV.sol,RankedBallot.sol,SchulzeBallot.sol,KemenyYoungBallot.solError Handling Improvements
Bug:
BallotGeneratorandResultCalculatorsilently defaulted toGeneralBalloton invalid typesFix: Now reverts with explicit
InvalidBallotTypeandInvalidResultTypeerrors🔐 Anonymous Voting Module Security Restoration
Authentication Routing (
App.js)Bug: Authentication routing was commented out, allowing unauthenticated access
Fix: Re-enabled authentication check and redirect to
/authfor unauthenticated usersAdmin Verification (
Election.js)Bug: Admin check was hardcoded to
true, bypassing authorizationFix: Now validates connected wallet against
electionOrganizerIdentity Verification Callback (
PolygonIDVerifier.js)Bug: Verification result callback was commented out
Fix: Re-enabled
onVerificationResult(result)callbackSocket Session Isolation (
index.js)Bug: Socket events were broadcast to all connected clients instead of specific sessions
Fix: Changed
io.sockets.emit()toio.to(sessionId).emit()for session-specific messagingMemory Leak Prevention (
index.js)Bug: Auth requests stored in memory were never cleaned up
Fix: Added 10-minute cleanup timeout for auth requests
Issuer Validation (
KYCAgeCredential.js)Bug:
allowedIssuers: ["*"]accepted credentials from any issuerFix: Set to empty array (reject by default) with TODO for production configuration
📜 Election Contract Hardening
Time Validation (
Election.sol,ElectionFactory.sol)InvalidElectionTimeerrorstartTime < endTimestartTime > block.timestampModifier Fix (
Election.sol)electionStarted→electionNotStarted>=instead of>to prevent operations at exact start timeRemove Candidate Fix (
Election.sol)Double-Result Prevention (
Election.sol)ResultsAlreadyDeclarederrorgetResult()multiple timesBallot-Result Compatibility (
ElectionFactory.sol)_validateBallotResultCompat()to ensure only compatible ballot/result type pairs🖥️ Client Fixes
Async Chain Switching Race Condition
Files:
ButtonCard.tsx,CandidateCard.tsx,CandidateGrid.tsx,CreditsVoting.tsx,CrossChain.tsxBug:
switchChain()was not awaited, causing contract calls to execute before chain switch completedFix: Changed to
await switchChainAsync()IPFS API Security
Bug: Pinata JWT was exposed client-side via
NEXT_PUBLIC_PINATA_JWTFix:
/api/ipfs/route.ts)pinToIPFS.tsto proxy through serverPINATA_JWT(server-only)GetUserElections Hook
Bug: Returned all elections instead of filtering by connected user
Fix: Now fetches election owners and filters where
owner === connectedAddressSSR Compatibility
Bug:
window.location.hrefaccessed during server-side renderFix: Added SSR guard and moved state update to
useEffectFiles Changed (28 files, +368/-213 lines)
Blockchain (13 files)
blockchain/contracts/Election.solblockchain/contracts/ElectionFactory.solblockchain/contracts/ballots/BallotGenerator.solblockchain/contracts/ballots/IRV.solblockchain/contracts/ballots/KemenyYoungBallot.solblockchain/contracts/ballots/QuadraticBallot.solblockchain/contracts/ballots/RankedBallot.solblockchain/contracts/ballots/SchulzeBallot.solblockchain/contracts/ballots/interface/Errors.solblockchain/contracts/resultCalculators/IRVResult.solblockchain/contracts/resultCalculators/KemenyYoungResult.solblockchain/contracts/resultCalculators/MooreResult.solblockchain/contracts/resultCalculators/ResultCalculator.solClient (10 files)
client/app/api/ipfs/route.ts(new)client/app/components/Cards/ButtonCard.tsxclient/app/components/Cards/VotingCards/CandidateCard.tsxclient/app/components/Cards/VotingCards/CandidateGrid.tsxclient/app/components/Cards/VotingCards/CreditsVoting.tsxclient/app/components/Helper/CrossChain.tsxclient/app/components/Hooks/GetUserElections.tsclient/app/election/[id]/page.tsxclient/app/helpers/client.tsclient/app/helpers/pinToIPFS.tsAnonymous Voting (5 files)
anonymousVoting/clientAnonymousVoting/src/App.jsanonymousVoting/clientAnonymousVoting/src/components/Election.jsanonymousVoting/clientAnonymousVoting/src/components/modals/PolygonIDVerifier.jsanonymousVoting/serverAnonymous/index.jsanonymousVoting/serverAnonymous/vcHelpers/KYCAgeCredential.jsstartTime >= endTimeorstartTime <= block.timestampwill revertTesting Checklist
Fixes Issue #237
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores