Skip to content

Fix critical security vulnerabilities in VouchMe contract #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nihallllll wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix critical security vulnerabilities in VouchMe contract #23

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d95720f
fix: resolve critical security vulnerabilities
Nihallllll Jan 16, 2026
26060b2
fix: resolve critical security vulnerabilities in VouchMe contract
Nihallllll Jan 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions contracts/src/VouchMe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import "@openzeppelin/contracts/utils/Strings.sol";
import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

contract VouchMe is ERC721URIStorage {
contract VouchMe is ERC721URIStorage, ReentrancyGuard {
using ECDSA for bytes32;
using Strings for uint256;

Expand Down Expand Up @@ -68,7 +69,7 @@ contract VouchMe is ERC721URIStorage {
string calldata giverName,
string calldata profileUrl,
bytes calldata signature
) external returns (uint256) {
) external nonReentrant returns (uint256) {
// Hash the message that was signed
bytes32 messageHash = keccak256(
abi.encodePacked(
Expand Down Expand Up @@ -275,6 +276,9 @@ contract VouchMe is ERC721URIStorage {
* @param receiver The receiver of the testimonial
*/
function _removeTestimonialFromList(uint256 tokenId, address sender, address receiver) internal {
// Delete testimonial data to fix memory leak
delete _testimonials[tokenId];

Comment on lines 278 to +281
Copy link

@coderabbitai coderabbitai bot Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

// Delete from testimonial mapping
delete _testimonial[sender][receiver];

Expand All @@ -301,7 +305,7 @@ contract VouchMe is ERC721URIStorage {
* @dev Deletes a testimonial
* @param tokenId The token ID to delete
*/
function deleteTestimonial(uint256 tokenId) external {
function deleteTestimonial(uint256 tokenId) external nonReentrant {
require(_ownerOf(tokenId) == msg.sender, "Only recipient can delete");

// Check if the testimonial still exists
Expand Down