Unnecessary Use of payable in NFT Transfers

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L129-L136

Vulnerability details

Impact

The transferFrom() and safeTransferFrom() function in the OwnershipNFTs contract are marked as payable, but there is no reason for them to accept Ether payments. This can confuse and lead to accidental Ether transfers.

Proof of Concept

Both functions are marked as payable

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
) external payable {
    // code
}

 function safeTransferFrom(
        address _from,
        address _to,
        uint256 _tokenId
    ) external payable {
        _transfer(_from, _to, _tokenId);
        _onTransferReceived(msg.sender, _from, _to, _tokenId);
    }

If a user mistakenly sends Ether during an NFT transfer, the Ether will be stuck in the contract with no way to retrieve it.

Tools Used

Manual

Recommended Mitigation Steps

Remove the payable modifier from the transferFrom() and safeTransferFrom() functions

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
) external {
    // code
}

This ensures that no Ether is sent with the NFT transfers.

Assessed type

Payable