Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
45 changes: 36 additions & 9 deletions src/IPNFT.sol
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,31 @@ contract IPNFT is ERC721URIStorageUpgradeable, ERC721BurnableUpgradeable, IReser
emit Reserved(_msgSender(), reservationId);
}

/**
* @notice mints an IPNFT with `tokenURI` as source of metadata. This IPNFT is linked a proof of idea (POI) which is a hash of any collection of files that represents an idea, anchored on any chain.
* @notice We are charging a nominal fee to symbolically represent the transfer of ownership rights, for a price of .001 ETH (<$2USD at current prices). This helps ensure the protocol is affordable to almost all projects, but discourages frivolous IP-NFT minting.
*
* @param to the recipient of the NFT
* @param poi the hash of the poi that will be computed to the tokenId
* @param _tokenURI a location that resolves to a valid IP-NFT metadata structure
* @param _symbol a symbol that represents the IPNFT's derivatives. Can be changed by the owner
* @param authorization a bytes encoded parameter that ensures that the poi is owned by the owner (to param)
* @return computedTokenId
*/
function mintWithPOI(address to, bytes32 poi, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization)
external
payable
whenNotPaused
returns (uint256)
{
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add reentrancy protection to mintWithPOI function

The mintWithPOI function is payable and involves external calls and state changes, which could be susceptible to reentrancy attacks.

To protect against reentrancy, consider using OpenZeppelin's ReentrancyGuardUpgradeable and applying the nonReentrant modifier to the function.

Implement as follows:

+import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

 contract IPNFT is ERC721URIStorageUpgradeable, ERC721BurnableUpgradeable, IReservable, UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable {
     // ...

     function initialize() external initializer {
         // ...
+        __ReentrancyGuard_init();
     }

     function mintWithPOI(address to, bytes32 poi, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization)
         external
         payable
+        nonReentrant
         whenNotPaused
         returns (uint256)
     {
         // ...
     }
     // ...
 }

Committable suggestion was skipped due to low confidence.

uint256 computedTokenId = uint256(poi);
_handleMint(to, computedTokenId, _tokenURI, _symbol, authorization);
return computedTokenId;
}

/**
* @notice mints an IPNFT with `tokenURI` as source of metadata. Invalidates the reservation. Redeems `mintpassId` on the authorizer contract
* @notice We are charging a nominal fee to symbolically represent the transfer of ownership rights, for a price of .001 ETH (<$2USD at current prices). This helps the ensure the protocol is affordable to almost all projects, but discourages frivolous IP-NFT minting.
* @notice We are charging a nominal fee to symbolically represent the transfer of ownership rights, for a price of .001 ETH (<$2USD at current prices). This helps ensure the protocol is affordable to almost all projects, but discourages frivolous IP-NFT minting.
*
* @param to the recipient of the NFT
* @param reservationId the reserved token id that has been reserved with `reserve()`
Expand All @@ -124,22 +146,27 @@ contract IPNFT is ERC721URIStorageUpgradeable, ERC721BurnableUpgradeable, IReser
revert NotOwningReservation(reservationId);
}


_handleMint(to, reservationId, _tokenURI, _symbol, authorization);
delete reservations[reservationId];
return reservationId;
}

function _handleMint(address to, uint256 tokenId, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization) internal {
if (msg.value < SYMBOLIC_MINT_FEE) {
revert MintingFeeTooLow();
}

if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(reservationId, _tokenURI, authorization)))) {
if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(tokenId, _tokenURI, authorization)))) {
revert Unauthorized();
}

delete reservations[reservationId];
symbol[reservationId] = _symbol;
symbol[tokenId] = _symbol;
mintAuthorizer.redeem(authorization);

_mint(to, reservationId);
_setTokenURI(reservationId, _tokenURI);
emit IPNFTMinted(to, reservationId, _tokenURI, _symbol);
return reservationId;
_mint(to, tokenId);
_setTokenURI(tokenId, _tokenURI);
emit IPNFTMinted(to, tokenId, _tokenURI, _symbol);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential reentrancy vulnerability in _handleMint function

The _handleMint function makes an external call to mintAuthorizer.redeem(authorization) before completing all state changes and emitting events. This could introduce a reentrancy vulnerability if the external contract is untrusted.

To mitigate this risk, consider reordering the operations to follow the checks-effects-interactions pattern. Perform all state changes and emit events before making external calls.

Apply this diff to rearrange the code:

 function _handleMint(address to, uint256 tokenId, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization) internal {
     if (msg.value < SYMBOLIC_MINT_FEE) {
         revert MintingFeeTooLow();
     }

     if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(tokenId, _tokenURI, authorization)))) {
         revert Unauthorized();
     }

+    // State changes
     symbol[tokenId] = _symbol;
     _mint(to, tokenId);
     _setTokenURI(tokenId, _tokenURI);
     emit IPNFTMinted(to, tokenId, _tokenURI, _symbol);

+    // External call after state changes
     mintAuthorizer.redeem(authorization);
 }
📝 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.

Suggested change
function _handleMint(address to, uint256 tokenId, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization) internal {
if (msg.value < SYMBOLIC_MINT_FEE) {
revert MintingFeeTooLow();
}
if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(reservationId, _tokenURI, authorization)))) {
if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(tokenId, _tokenURI, authorization)))) {
revert Unauthorized();
}
delete reservations[reservationId];
symbol[reservationId] = _symbol;
symbol[tokenId] = _symbol;
mintAuthorizer.redeem(authorization);
_mint(to, reservationId);
_setTokenURI(reservationId, _tokenURI);
emit IPNFTMinted(to, reservationId, _tokenURI, _symbol);
return reservationId;
_mint(to, tokenId);
_setTokenURI(tokenId, _tokenURI);
emit IPNFTMinted(to, tokenId, _tokenURI, _symbol);
function _handleMint(address to, uint256 tokenId, string calldata _tokenURI, string calldata _symbol, bytes calldata authorization) internal {
if (msg.value < SYMBOLIC_MINT_FEE) {
revert MintingFeeTooLow();
}
if (!mintAuthorizer.authorizeMint(_msgSender(), to, abi.encode(SignedMintAuthorization(tokenId, _tokenURI, authorization)))) {
revert Unauthorized();
}
// State changes
symbol[tokenId] = _symbol;
_mint(to, tokenId);
_setTokenURI(tokenId, _tokenURI);
emit IPNFTMinted(to, tokenId, _tokenURI, _symbol);
// External call after state changes
mintAuthorizer.redeem(authorization);
}

}

/**
Expand Down Expand Up @@ -188,7 +215,7 @@ contract IPNFT is ERC721URIStorageUpgradeable, ERC721BurnableUpgradeable, IReser
(bool success,) = _msgSender().call{ value: address(this).balance }("");
require(success, "transfer failed");
}

/// @inheritdoc UUPSUpgradeable
function _authorizeUpgrade(address /*newImplementation*/ )
internal
Expand Down
58 changes: 58 additions & 0 deletions subgraph/abis/IPNFT.json
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,45 @@
],
"stateMutability": "payable"
},
{
"type": "function",
"name": "mintWithPOI",
"inputs": [
{
"name": "to",
"type": "address",
"internalType": "address"
},
{
"name": "poi",
"type": "bytes",
"internalType": "bytes"
},
{
"name": "_tokenURI",
"type": "string",
"internalType": "string"
},
{
"name": "_symbol",
"type": "string",
"internalType": "string"
},
{
"name": "authorization",
"type": "bytes",
"internalType": "bytes"
}
],
"outputs": [
{
"name": "",
"type": "uint256",
"internalType": "uint256"
}
],
"stateMutability": "payable"
},
{
"type": "function",
"name": "name",
Expand Down Expand Up @@ -721,6 +760,25 @@
],
"anonymous": false
},
{
"type": "event",
"name": "IPNFTPOI",
"inputs": [
{
"name": "tokenId",
"type": "uint256",
"indexed": true,
"internalType": "uint256"
},
{
"name": "poi",
"type": "bytes",
"indexed": false,
"internalType": "bytes"
}
],
"anonymous": false
},
{
"type": "event",
"name": "Initialized",
Expand Down
38 changes: 21 additions & 17 deletions subgraph/src/ipnftMapping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,21 @@ export function handleReservation(event: ReservedEvent): void {
reservation.save()
}

function updateIpnftMetadata(ipnft: Ipnft, uri: string, timestamp: BigInt): void {
let ipfsLocation = uri.replace('ipfs://', '');
if (!ipfsLocation || ipfsLocation == uri) {
log.error("Invalid URI format for tokenId {}: {}", [ipnft.id, uri])
return
}
function updateIpnftMetadata(
ipnft: Ipnft,
uri: string,
timestamp: BigInt
): void {
let ipfsLocation = uri.replace('ipfs://', '')
if (!ipfsLocation || ipfsLocation == uri) {
log.error('Invalid URI format for tokenId {}: {}', [ipnft.id, uri])
return
}

ipnft.tokenURI = uri
ipnft.metadata = ipfsLocation
ipnft.updatedAtTimestamp = timestamp
IpnftMetadataTemplate.create(ipfsLocation)
ipnft.tokenURI = uri
ipnft.metadata = ipfsLocation
ipnft.updatedAtTimestamp = timestamp
IpnftMetadataTemplate.create(ipfsLocation)
}

//the underlying parameter arrays are misaligned, hence we cannot cast or unify both events
Expand All @@ -97,7 +101,6 @@ export function handleMint(event: IPNFTMintedEvent): void {
updateIpnftMetadata(ipnft, event.params.tokenURI, event.block.timestamp)
store.remove('Reservation', event.params.tokenId.toString())
ipnft.save()

}

export function handleMetadataUpdated(event: MetadataUpdateEvent): void {
Expand All @@ -108,13 +111,14 @@ export function handleMetadataUpdated(event: MetadataUpdateEvent): void {
}

//erc4906 is not emitting the new url, we must query it ourselves
let _ipnftContract = IPNFTContract.bind(event.params._event.address);
let _ipnftContract = IPNFTContract.bind(event.params._event.address)
let newUri = _ipnftContract.tokenURI(event.params._tokenId)
if (!newUri || newUri == "") {
log.debug("no new uri found for token, likely just minted {}", [event.params._tokenId.toString()])
return
if (!newUri || newUri == '') {
log.debug('no new uri found for token, likely just minted {}', [
event.params._tokenId.toString()
])
return
}
updateIpnftMetadata(ipnft, newUri, event.block.timestamp)
updateIpnftMetadata(ipnft, newUri, event.block.timestamp)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding URI validation before metadata update.

While updateIpnftMetadata handles invalid URIs internally, adding validation before the update would be more efficient and provide clearer error handling.

Consider this improvement:

+  if (!newUri.startsWith('ipfs://')) {
+    log.error('Invalid URI format received in MetadataUpdate for token {}: {}', [
+      event.params._tokenId.toString(),
+      newUri
+    ])
+    return
+  }
   updateIpnftMetadata(ipnft, newUri, event.block.timestamp)
   ipnft.save()

Committable suggestion was skipped due to low confidence.

ipnft.save()
}

40 changes: 35 additions & 5 deletions test/IPNFT.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,39 @@ contract IPNFTTest is IPNFTMintHelper {
assertEq(ipnft.reservations(2), bob);
}

function testMintWithPoi() public {
bytes32 poiHash = 0x073cb54264ef688e56531a2d09ab47b14086b5c7813e3a23a2bd7b1bb6458a52;
uint256 tokenId = uint256(poiHash);
(, uint256 maliciousSignerPk) = makeAddrAndKey("malicious");
bytes32 authMessageHash = ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(alice, alice, tokenId, ipfsUri)));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(maliciousSignerPk, authMessageHash);
bytes memory maliciousAuthorization = abi.encodePacked(r, s, v);

vm.startPrank(deployer);
ipnft.setAuthorizer(new SignedMintAuthorizer(deployer));
vm.stopPrank();

vm.startPrank(alice);
vm.expectRevert(IPNFT.MintingFeeTooLow.selector);
ipnft.mintWithPOI(alice, poiHash, ipfsUri, DEFAULT_SYMBOL, maliciousAuthorization);

vm.expectRevert(IPNFT.Unauthorized.selector);
ipnft.mintWithPOI{ value: MINTING_FEE }(alice, poiHash, ipfsUri, DEFAULT_SYMBOL, maliciousAuthorization);

(v, r, s) = vm.sign(deployerPk, authMessageHash);
bytes memory authorization = abi.encodePacked(r, s, v);
vm.expectEmit(true, true, false, true);
emit IPNFTMinted(alice, tokenId, ipfsUri, DEFAULT_SYMBOL);
ipnft.mintWithPOI{ value: MINTING_FEE }(
alice, poiHash, ipfsUri, DEFAULT_SYMBOL, authorization
);
assertEq(ipnft.ownerOf(tokenId), alice);
assertEq(ipnft.tokenURI(tokenId), ipfsUri);
assertEq(ipnft.symbol(tokenId), DEFAULT_SYMBOL);
assertEq(tokenId, 3273451770044532981553402679345217193568252544895634663440128735015952812626);
vm.stopPrank();
}

function testMintFromReservation() public {
vm.startPrank(deployer);
ipnft.setAuthorizer(new SignedMintAuthorizer(deployer));
Expand Down Expand Up @@ -145,7 +178,6 @@ contract IPNFTTest is IPNFTMintHelper {
/**
* ... but when set as heir of a self destruct operation the contract accepts the money.
*/

function testOwnerCanWithdrawEthFunds() public {
vm.deal(address(bob), 10 ether);
vm.startPrank(bob);
Expand Down Expand Up @@ -232,15 +264,13 @@ contract IPNFTTest is IPNFTMintHelper {
//the signoff only allows alice to call this
vm.startPrank(charlie);
vm.expectRevert(IPNFT.Unauthorized.selector);
ipnft.amendMetadata(1, "ipfs://QmNewUri", authorization);
ipnft.amendMetadata(1, "ipfs://QmNewUri", authorization);

vm.startPrank(alice);
vm.expectEmit(true, true, false, false);
emit MetadataUpdate(1);
ipnft.amendMetadata(1, "ipfs://QmNewUri", authorization);
ipnft.amendMetadata(1, "ipfs://QmNewUri", authorization);
assertEq(ipnft.tokenURI(1), "ipfs://QmNewUri");
vm.stopPrank();
}


}