Fix: Add pagination to getReceivedTestimonials to prevent DoS from un…#33
Fix: Add pagination to getReceivedTestimonials to prevent DoS from un…#33Rav1Chauhan wants to merge 4 commits intoStabilityNexus:mainfrom
Conversation
…bounded array return
📝 WalkthroughWalkthroughAdded two public accessors for received testimonials: a count getter and a paginated retrieval function. Most other edits are formatting/multiline refactors and minor tokenURI concatenation reformatting; core token behavior and testimonial logic remain unchanged. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (3)
contracts/src/VouchMe.sol (3)
156-160:getReceivedTestimonialsCountis a duplicate of the existinggetTestimonialCount.Both functions are
external viewand return_receivedTestimonials[receiver].length— they are semantically identical. This inflates the ABI surface and creates confusion for integrators about which function to use.If the intent is to align naming with the new paginated getter, deprecate
getTestimonialCount(or remove it if there are no existing off-chain consumers) rather than keeping both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/VouchMe.sol` around lines 156 - 160, The two functions getReceivedTestimonialsCount and getTestimonialCount both return _receivedTestimonials[receiver].length and are duplicates; remove one to avoid ABI/API confusion or mark the older one deprecated. Choose which name to keep (suggest keeping the paginated-friendly getReceivedTestimonialsCount), delete the other function declaration (or add a deprecation comment and an event if you must preserve ABI compatibility), and update any internal references/tests to call the retained function name.
161-165: Add NatSpec to the new paginated getter and documentlimitsemantics.Both new functions (
getReceivedTestimonialsCountandgetReceivedTestimonials) are missing NatSpec@dev/@param/@returncomments, unlike the rest of the contract's public API. At minimum, document:
- that
startis zero-based,- that the returned slice may be shorter than
limitwhen near the end of the array,- that a caller passing
limit = 0receives an empty array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/VouchMe.sol` around lines 161 - 165, Add NatSpec comments to both getReceivedTestimonialsCount and getReceivedTestimonials: include an `@dev` describing that these are paginated getters, document `@param` start as zero-based index, `@param` limit as maximum number of items to return and that passing limit = 0 yields an empty array, and document `@return` that the returned array is a slice which may be shorter than limit when near the end of the stored testimonials; ensure the comments use `@param` for receiver, start, limit and `@return` for the uint256[] result and match the style of the contract's existing public API.
375-388:deleteTestimonialdoes not call_burn, leaving the NFT live on-chain.After
_removeTestimonialFromListruns, the internal accounting (mappings, arrays) is cleaned up, butownerOf(tokenId)still resolves to the receiver andtokenURI(tokenId)remains accessible. This is a pre-existing gap not introduced by this PR, but worth tracking: any downstream pagination client querying a token ID returned before deletion will still see a valid NFT despite the testimonial being "deleted" from the contract's perspective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/VouchMe.sol` around lines 375 - 388, deleteTestimonial currently clears internal testimonial mappings via _removeTestimonialFromList but does not destroy the ERC721 token, so ownerOf(tokenId) and tokenURI(tokenId) remain valid; after the require checks and after calling _removeTestimonialFromList in deleteTestimonial, invoke the ERC721 burn routine (call _burn(tokenId)) to remove the NFT from circulation and ensure ownerOf/tokenURI no longer resolve, keeping the existing require on _ownerOf(tokenId) and existing emit TestimonialDeleted(tokenId, msg.sender).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/VouchMe.sol`:
- Line 169: Replace the invalid dynamic array allocation "return new uint256;"
with a proper memory array allocation like "return new uint256[](0)" in the
function that currently returns an empty uint256[]; also remove the duplicated
function getReceivedTestimonialsCount() (or getTestimonialCount()) so only one
function returns _receivedTestimonials[receiver].length to avoid redundancy —
locate and update the functions named getReceivedTestimonialsCount and
getTestimonialCount and the return statement referencing new uint256 to apply
these changes.
---
Nitpick comments:
In `@contracts/src/VouchMe.sol`:
- Around line 156-160: The two functions getReceivedTestimonialsCount and
getTestimonialCount both return _receivedTestimonials[receiver].length and are
duplicates; remove one to avoid ABI/API confusion or mark the older one
deprecated. Choose which name to keep (suggest keeping the paginated-friendly
getReceivedTestimonialsCount), delete the other function declaration (or add a
deprecation comment and an event if you must preserve ABI compatibility), and
update any internal references/tests to call the retained function name.
- Around line 161-165: Add NatSpec comments to both getReceivedTestimonialsCount
and getReceivedTestimonials: include an `@dev` describing that these are paginated
getters, document `@param` start as zero-based index, `@param` limit as maximum
number of items to return and that passing limit = 0 yields an empty array, and
document `@return` that the returned array is a slice which may be shorter than
limit when near the end of the stored testimonials; ensure the comments use
`@param` for receiver, start, limit and `@return` for the uint256[] result and match
the style of the contract's existing public API.
- Around line 375-388: deleteTestimonial currently clears internal testimonial
mappings via _removeTestimonialFromList but does not destroy the ERC721 token,
so ownerOf(tokenId) and tokenURI(tokenId) remain valid; after the require checks
and after calling _removeTestimonialFromList in deleteTestimonial, invoke the
ERC721 burn routine (call _burn(tokenId)) to remove the NFT from circulation and
ensure ownerOf/tokenURI no longer resolve, keeping the existing require on
_ownerOf(tokenId) and existing emit TestimonialDeleted(tokenId, msg.sender).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/src/VouchMe.sol (1)
156-160: 🛠️ Refactor suggestion | 🟠 MajorDuplicate function:
getReceivedTestimonialsCountduplicatesgetTestimonialCount.Both functions return
_receivedTestimonials[receiver].length. Remove one to avoid confusion and reduce contract bytecode size. Consider keepinggetTestimonialCount(lines 213-217) since it already exists and deprecating this new one, or vice versa with a clear naming convention for the pagination API.♻️ Suggested fix: Remove the duplicate function
- function getReceivedTestimonialsCount( - address receiver - ) external view returns (uint256) { - return _receivedTestimonials[receiver].length; - }Then use
getTestimonialCount(receiver)consistently for pagination, or renamegetTestimonialCounttogetReceivedTestimonialsCountand remove the old one if you prefer the new naming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/VouchMe.sol` around lines 156 - 160, Remove the duplicate getter to avoid confusion and reduce bytecode: delete the getReceivedTestimonialsCount function and use the existing getTestimonialCount function (which returns _receivedTestimonials[receiver].length) consistently across the contract and pagination API, or alternatively keep getReceivedTestimonialsCount and remove/rename getTestimonialCount—ensure only one of the two remains and update any external references to the retained function name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/VouchMe.sol`:
- Around line 161-185: getReceivedTestimonials was changed to a paginated
signature (address receiver, uint256 start, uint256 limit) which breaks existing
callers and the ABI; update all test calls that still pass a single address to
call getReceivedTestimonials(receiver, 0, <suitable_limit>) (e.g., use total
length or a safe max like 100) and regenerate the contract ABI so
web/src/abis/VouchMe.json matches the new function signature; search for
references to getReceivedTestimonials in tests (e.g., VouchMe.t.sol and
VouchMeFuzz.t.sol) and replace the single-arg calls accordingly, then recompile
and refresh the ABI artifact.
---
Duplicate comments:
In `@contracts/src/VouchMe.sol`:
- Around line 156-160: Remove the duplicate getter to avoid confusion and reduce
bytecode: delete the getReceivedTestimonialsCount function and use the existing
getTestimonialCount function (which returns
_receivedTestimonials[receiver].length) consistently across the contract and
pagination API, or alternatively keep getReceivedTestimonialsCount and
remove/rename getTestimonialCount—ensure only one of the two remains and update
any external references to the retained function name.
| function getReceivedTestimonials( | ||
| address receiver, | ||
| uint256 start, | ||
| uint256 limit | ||
| ) external view returns (uint256[] memory) { | ||
| uint256 total = _receivedTestimonials[receiver].length; | ||
|
|
||
| if (start >= total) { | ||
| return new uint256[](0); | ||
| } | ||
|
|
||
| uint256 end = start + limit; | ||
| if (end > total) { | ||
| end = total; | ||
| } | ||
|
|
||
| uint256 size = end - start; | ||
| uint256[] memory result = new uint256[](size); | ||
|
|
||
| for (uint256 i = 0; i < size; i++) { | ||
| result[i] = _receivedTestimonials[receiver][start + i]; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the VouchMe.sol contract to see all signatures of getReceivedTestimonials
rg -n "function getReceivedTestimonials" contracts/src/VouchMe.sol -A 10Repository: StabilityNexus/VouchMe
Length of output: 599
🏁 Script executed:
# Check for the old single-argument signature anywhere in the contract
rg -n "getReceivedTestimonials\s*\(" contracts/src/VouchMe.sol -B 2 -A 8Repository: StabilityNexus/VouchMe
Length of output: 460
🏁 Script executed:
# Check test file at line 290
sed -n '285,295p' contracts/test/VouchMe.t.solRepository: StabilityNexus/VouchMe
Length of output: 513
🏁 Script executed:
# Check VouchMeFuzz test file at lines 94 and 177
sed -n '90,98p' contracts/test/VouchMeFuzz.t.sol
sed -n '173,181p' contracts/test/VouchMeFuzz.t.solRepository: StabilityNexus/VouchMe
Length of output: 765
🏁 Script executed:
# Check for the getTestimonialCount function mentioned in the suggested fix
rg -n "function getTestimonialCount" contracts/src/VouchMe.sol -A 5Repository: StabilityNexus/VouchMe
Length of output: 250
🏁 Script executed:
# Check ABI file for the function signature
rg -n "getReceivedTestimonials" web/src/abis/VouchMe.json -B 2 -A 10Repository: StabilityNexus/VouchMe
Length of output: 28963
Pagination implementation is correct, but this is a breaking change that will cause test compilation failures.
The pagination logic properly bounds gas usage and addresses the DoS concern. However, the new signature requires three parameters (receiver, start, limit), breaking existing callers that pass only the receiver address:
contracts/test/VouchMe.t.sol:290callsgetReceivedTestimonials(alice)contracts/test/VouchMeFuzz.t.sol:94and:177callgetReceivedTestimonials(receiver)web/src/abis/VouchMe.jsonhas stale ABI entries for the old single-argument signature
🔧 Suggested test file updates
For tests, update calls to use the new paginated signature. Example for VouchMe.t.sol:290:
-uint256[] memory remainingTestimonials = vouchMe.getReceivedTestimonials(alice);
+uint256 count = vouchMe.getTestimonialCount(alice);
+uint256[] memory remainingTestimonials = vouchMe.getReceivedTestimonials(alice, 0, count);Also regenerate the ABI file after contract changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/VouchMe.sol` around lines 161 - 185, getReceivedTestimonials
was changed to a paginated signature (address receiver, uint256 start, uint256
limit) which breaks existing callers and the ABI; update all test calls that
still pass a single address to call getReceivedTestimonials(receiver, 0,
<suitable_limit>) (e.g., use total length or a safe max like 100) and regenerate
the contract ABI so web/src/abis/VouchMe.json matches the new function
signature; search for references to getReceivedTestimonials in tests (e.g.,
VouchMe.t.sol and VouchMeFuzz.t.sol) and replace the single-arg calls
accordingly, then recompile and refresh the ABI artifact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/test/VouchMe.t.sol (1)
473-480: Consider testing withgetTestimonialCountfor consistency.The test passes
limit=0directly. While this works (sincestart >= totalreturns empty), it doesn't explicitly verify the "no testimonials exist" scenario the same way other tests do.For consistency with other tests and clearer intent, consider:
function testGetReceivedTestimonialsEmpty() public { + uint256 count = vouchMe.getTestimonialCount(alice); + assertEq(count, 0); // Explicitly verify no testimonials exist uint256[] memory testimonials = vouchMe.getReceivedTestimonials( alice, 0, - 0 + count ); assertEq(testimonials.length, 0); }This is a minor style suggestion—the current test is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/VouchMe.t.sol` around lines 473 - 480, Replace the hardcoded zero in testGetReceivedTestimonialsEmpty with an explicit count fetched from the contract: call vouchMe.getTestimonialCount(alice) into a uint256 total and then call vouchMe.getReceivedTestimonials(alice, total, 0) (or start = total, limit = 0) and assert length == 0; this uses getTestimonialCount to clearly express "no testimonials exist" like other tests and keeps the assertion behavior the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Line 1: The repository contains a committed `.env` exposing
NEXT_PUBLIC_PROJECT_ID; remove this credential from the repo and history and
prevent future leaks: add a `.gitignore` entry for `.env`, create a
`.env.example` with a placeholder NEXT_PUBLIC_PROJECT_ID to document required
variables, rotate the exposed WalletConnect Project ID in the WalletConnect
Cloud dashboard, delete the committed `.env` from the repository and purge it
from git history using a history-rewriting tool (e.g., BFG or git
filter-branch), and ensure the new actual `.env` is not tracked going forward.
---
Nitpick comments:
In `@contracts/test/VouchMe.t.sol`:
- Around line 473-480: Replace the hardcoded zero in
testGetReceivedTestimonialsEmpty with an explicit count fetched from the
contract: call vouchMe.getTestimonialCount(alice) into a uint256 total and then
call vouchMe.getReceivedTestimonials(alice, total, 0) (or start = total, limit =
0) and assert length == 0; this uses getTestimonialCount to clearly express "no
testimonials exist" like other tests and keeps the assertion behavior the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd7ceb6d-2e54-407e-a450-cc8760cbbfef
📒 Files selected for processing (2)
.envcontracts/test/VouchMe.t.sol
.env
Outdated
| @@ -0,0 +1 @@ | |||
| NEXT_PUBLIC_PROJECT_ID=38b236298d3253f98d42e08991ad74d3 No newline at end of file | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .env is in .gitignore
echo "Checking .gitignore for .env patterns:"
if [ -f .gitignore ]; then
grep -n "\.env" .gitignore || echo "No .env pattern found in .gitignore"
else
echo ".gitignore file not found"
fi
# Check for .env.example
echo -e "\nChecking for .env.example:"
fd -t f ".env.example" --max-depth 2
# Check if there are other .env files committed
echo -e "\nOther .env files in repository:"
fd -t f "^\.env" --hiddenRepository: StabilityNexus/VouchMe
Length of output: 196
Do not commit .env files with credentials to version control.
The .env file with your WalletConnect Project ID is committed to the repository. Even though NEXT_PUBLIC_ credentials are publicly visible at runtime, committing them creates security risks:
- The Project ID can be abused for quota exhaustion or project impersonation
- Git history permanently associates this credential with your project
- No
.gitignorecurrently prevents accidental commits of sensitive files
Required actions:
- Create
.gitignoreand add.envto it - Create
.env.examplewith a placeholder value for documentation - Rotate the exposed Project ID in WalletConnect Cloud
- Remove this file from git history using
git filter-branchor BFG Repo-Cleaner
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 1-1: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env at line 1, The repository contains a committed `.env` exposing
NEXT_PUBLIC_PROJECT_ID; remove this credential from the repo and history and
prevent future leaks: add a `.gitignore` entry for `.env`, create a
`.env.example` with a placeholder NEXT_PUBLIC_PROJECT_ID to document required
variables, rotate the exposed WalletConnect Project ID in the WalletConnect
Cloud dashboard, delete the committed `.env` from the repository and purge it
from git history using a history-rewriting tool (e.g., BFG or git
filter-branch), and ensure the new actual `.env` is not tracked going forward.
1 participant
…bounded array return
Addressed Issues:
Fixes #28
Fix: Prevent DoS via Unbounded Array Return in getReceivedTestimonials
Problem
The previous implementation of:
function getReceivedTestimonials(address receiver)
returned the entire _receivedTestimonials[receiver] dynamic array.
As testimonials accumulate over time, the array grows without bound.
Solidity must copy the entire array from storage to memory when returning it.
This causes:
Gas usage growing linearly with array size
Potential exceeding of block gas limit
Function reverting due to out-of-gas
Inability for users/dApps to retrieve testimonials
Possible Denial of Service (DoS) scenario
Solution
Replaced the unbounded return with pagination:
Added:
getReceivedTestimonialsCount(address receiver)
getReceivedTestimonials(address receiver, uint256 start, uint256 limit)
This ensures:
Bounded gas usage
Safe and scalable retrieval
No change to storage logic
Backward compatibility with existing data
Impact
Prevents potential DoS
Improves scalability
Aligns with Solidity best practices
Screenshots/Recordings:
This change introduces pagination to prevent potential Denial of Service (DoS) caused by returning an unbounded dynamic array.
The previous implementation returned the entire _receivedTestimonials array for a user, which could grow indefinitely over time. As Solidity copies storage arrays to memory when returning them, gas costs grow linearly with array size. Eventually, this could cause the function to revert due to exceeding the block gas limit.
The updated implementation adds:
This ensures scalable and gas-bounded retrieval while preserving existing storage logic..
Additional Notes:
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Chores