QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

Unsafe ERC721 Transfer Implementation in LPNFT Contract Could Lead to Permanently Locked Tokens

0xshoonya

Summary

The LPNFT contract, which represents liquidity positions in the QuantAMM protocol, inherits from OpenZeppelin's ERC721 but does not enforce the use of safeTransferFrom when transferring tokens to contract addresses. This could result in LP tokens becoming permanently locked if transferred to a contract that doesn't implement the ERC721Receiver interface.

Vulnerability Details

The LPNFT contract currently allows both transferFrom and safeTransferFrom methods for token transfers, as inherited from OpenZeppelin's ERC721 implementation. While it overrides the internal _update function to handle vault balance updates, it doesn't add any safety checks for contract recipients:
LPNFT.sol#L49-L56

function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
previousOwner = super._update(to, tokenId, auth);
//_update is called during mint, burn and transfer. This functionality is only for transfer
if (to != address(0) && previousOwner != address(0)) {
//if transfering the record in the vault needs to be changed to reflect the change in ownership
router.afterUpdate(previousOwner, to, tokenId);
}
}

The vulnerability arises because:

  1. The contract doesn't override the external transferFrom function to prevent transfers to non-receiving contracts

  2. Users can freely use transferFrom instead of safeTransferFrom

  3. The _update function focuses on vault record updates but doesn't check if the recipient can handle ERC721 tokens

  4. The contract represents valuable LP positions, making token recovery impossible if they get stuck

Impact

Permanent loss of LP tokens if transferred to non-receiving contracts

Tools Used

Manual Review

Recommendations

Modify the _update function to include safety checks:

function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
if (to.code.length > 0) {
try IERC721Receiver(to).onERC721Received(msg.sender, auth, tokenId, "") returns (bytes4 retval) {
if (retval != IERC721Receiver.onERC721Received.selector) {
revert("ERC721: invalid receiver");
}
} catch {
revert("ERC721: transfer to non-ERC721Receiver");
}
}
previousOwner = super._update(to, tokenId, auth);
if (to != address(0) && previousOwner != address(0)) {
router.afterUpdate(previousOwner, to, tokenId);
}
}
Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas / Admin is trusted / Pool creation is trusted / User mistake / Suppositions

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.