Fix critical security vulnerabilities in VouchMe contract

[Nihallllll]

Summary

This PR addresses critical security vulnerabilities discovered in the VouchMe smart contract.

-Added Reentrancyguard in createTestimonial and deleteTestimonial

-Added delete _testimonials[tokenId]

Impact

• Frees up storage space
• Reduces gas costs over time
• Gets gas refund on deletion
• safe after adding reentrancyguard

Summary by CodeRabbit

• Security Enhancements
  • Reinforced protection mechanisms for testimonial creation and deletion operations against potential attack vectors.
  • Optimized memory cleanup procedures during testimonial removal to enhance contract efficiency and performance.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The VouchMe contract now extends ReentrancyGuard and applies the nonReentrant modifier to createTestimonial and deleteTestimonial functions for reentrancy protection. Memory cleanup was added to _removeTestimonialFromList to delete testimonial state before removal.

Changes

Cohort / File(s)	Summary
Reentrancy Protection contracts/src/VouchMe.sol	Added ReentrancyGuard inheritance to contract; applied nonReentrant modifier to createTestimonial and deleteTestimonial functions; added explicit memory cleanup (_testimonials[tokenId] deletion) in _removeTestimonialFromList.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A guard for the guard, protection most fair,
Reentrancy dangers? The contract won't spare!
With modifiers applied and memory swept clean,
The safest testimonials you've ever seen! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly reflects the main changes: adding ReentrancyGuard to protect against reentrancy attacks and implementing storage cleanup.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@contracts/src/VouchMe.sol`:
- Around line 278-281: The delete in _removeTestimonialFromList only clears the
_testimonials[tokenId] struct but leaves ERC721 ownership and tokenURI data
(ERC721._owners and ERC721URIStorage._tokenURIs) intact; change
_removeTestimonialFromList to burn the NFT (call _burn(tokenId)) after deleting
testimonial data so ownership and tokenURI storage are cleared, and update the
_update function (or transfer blocker) to permit burns while still preventing
transfers by allowing _burn to bypass the transfer-blocking check; ensure you
reference and use the existing _burn method from ERC721/ERC721URIStorage and
adjust any transfer guard that previously blocked burns so burns succeed but
normal transfers remain blocked.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56c2737 and 26060b2.

📒 Files selected for processing (1)

• contracts/src/VouchMe.sol

🔇 Additional comments (1)

contracts/src/VouchMe.sol (1)

8-10: The import path @openzeppelin/contracts/utils/ReentrancyGuard.sol is correct for OpenZeppelin v5. OpenZeppelin v4 uses security/ while v5 uses utils/. Since the code uses the v5 path, the import will compile correctly with OpenZeppelin v5.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "VouchMe.sol" | head -5

Repository: StabilityNexus/VouchMe

Length of output: 93

---

🏁 Script executed:

cat -n contracts/src/VouchMe.sol | head -100

Repository: StabilityNexus/VouchMe

Length of output: 4659

---

🏁 Script executed:

cat -n contracts/src/VouchMe.sol | sed -n '270,330p'

Repository: StabilityNexus/VouchMe

Length of output: 2354

---

🏁 Script executed:

# Check the full _removeTestimonialFromList and surrounding context
cat -n contracts/src/VouchMe.sol | sed -n '275,320p'

Repository: StabilityNexus/VouchMe

Length of output: 2170

---

🏁 Script executed:

# Check _update method
rg -n "_update" contracts/src/VouchMe.sol -A 5

Repository: StabilityNexus/VouchMe

Length of output: 420

---

🏁 Script executed:

# Check the imports to understand ERC721 inheritance
head -50 contracts/src/VouchMe.sol

Repository: StabilityNexus/VouchMe

Length of output: 1893

---

Deletion leaves ERC721 ownership and tokenURI queryable after removal.

delete _testimonials[tokenId] only clears the testimonial struct. ERC721URIStorage's _tokenURIs mapping and ERC721's _owners mapping remain intact, so the token still exists as an NFT, its URI is still queryable, and storage isn't fully reclaimed. To achieve complete cleanup, burn the token and modify _update to allow burns while blocking transfers.

🧹 Suggested fix (burn + allow burns while blocking transfers)

 function _removeTestimonialFromList(uint256 tokenId, address sender, address receiver) internal {
     // Delete testimonial data to fix memory leak
     delete _testimonials[tokenId];
+    // Burn to clear ownership + URI storage (gas refund)
+    _burn(tokenId);

@@
 function _update(address to, uint256 tokenId, address auth) internal override returns (address) {
-    require(_ownerOf(tokenId) == address(0), "Tokens are non-transferrable");
+    address from = _ownerOf(tokenId);
+    // Allow mint (from == 0) and burn (to == 0); block transfers
+    require(from == address(0) || to == address(0), "Tokens are non-transferrable");
     return super._update(to, tokenId, auth);
 }

Also applies to: 308-318

🤖 Prompt for AI Agents

In `@contracts/src/VouchMe.sol` around lines 278 - 281, The delete in
_removeTestimonialFromList only clears the _testimonials[tokenId] struct but
leaves ERC721 ownership and tokenURI data (ERC721._owners and
ERC721URIStorage._tokenURIs) intact; change _removeTestimonialFromList to burn
the NFT (call _burn(tokenId)) after deleting testimonial data so ownership and
tokenURI storage are cleared, and update the _update function (or transfer
blocker) to permit burns while still preventing transfers by allowing _burn to
bypass the transfer-blocking check; ensure you reference and use the existing
_burn method from ERC721/ERC721URIStorage and adjust any transfer guard that
previously blocked burns so burns succeed but normal transfers remain blocked.