NFT Dealers

First Flight #58
Beginner FriendlyFoundry
100 EXP
Submission Details
Impact: low
Likelihood: medium

`buy()` and `mint()` functions doesn't need to be `payable`

Author Revealed upon completion

Root + Impact

Description

  • The protocol is exchanging NFTs with ERC20 USDC tokens and not with the native blockchain tokens.

  • With buy() function being payable, this function accepts msg.value. There could be innocent users that transfer token value to this function and the amount is lost forever.

// Root cause in the codebase with @> marks to highlight the relevant
@> function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
//...
}
@> function buy(uint256 _listingId) external payable {
//...
}

Risk

Likelihood:

  • A user calls buy() or mintNft() with msg.value set. In that case, both his value and USDC balance will be deducted and transfer to the protocol smart contract.

Impact:

  • Incorrect behavior and innocent buyers could forever lose the native token balance they transfer when making buy() call.

  • There is also no withdraw() function in the smart contract to withdraw the transferred native balance. So they are forever lost.

Proof of Concept

The user directly transfer native blockchain token when calling buy() function and lose its value forever.

vm.prank(user);
nftDealers.mint{value: 10000}();
nftDealers.buy{value: 10000}(1);

Recommended Mitigation

Remove the payable from the two functions mintNft() and buy(). Of course we can also add a withdraw function for the owner to withdraw native token balance and left the payable intact. But this is not fair to users who carelessly transfer native balance to the contract.

- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external onlyWhenRevealed onlyWhitelisted {
- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external {
//...
}

Support

FAQs

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

Give us feedback!