b0g0_ctf - Insecure use of _mint in BuggyNFTVault contract

[Viktor-Andreev4]

Bug Report: Insecure use of _mint in BuggyNFTVault contract

Description of the Bug:

The BuggyNFTVault contract currently uses the _mint function to mint ERC721 tokens when a user deposits ETH. The _mint function directly transfers the token to the recipient without verifying if the recipient is a smart contract and whether it is capable of handling ERC721 tokens. This presents two main risks:

Reentrancy Risk:

• If the recipient of the token is a smart contract that does not implement the onERC721Received function, the token is transferred, but the receiving contract may not be able to handle it properly. This can potentially open up a reentrancy attack vector, where the receiving contract could exploit the _mint function to execute unintended behaviors or reenter the minting process.

Token Loss Risk:

• If the recipient contract does not implement onERC721Received or handles it incorrectly, the minted token could be lost or become irretrievable, as the recipient contract might not have the necessary logic to manage the incoming token.

  Impact:

Reentrancy Exploit:

A malicious contract could potentially use the token received during the _mint process to reenter the contract and execute unintended operations, leading to potential fund loss or other security breaches.

Lost Tokens:

ERC721 tokens could be sent to smart contracts that are not equipped to manage them, resulting in those tokens being stuck or permanently lost.

Solution:

To mitigate these risks, the contract should use the _safeMint function instead of _mint. The _safeMint function performs additional checks to ensure that if the recipient is a smart contract, it implements the onERC721Received function. This function returns a specific selector to confirm that the contract is capable of handling ERC721 tokens.

Here’s how the code can be adjusted:

_safeMint(msg.sender, newTokenId);

By using _safeMint, you ensure:

Safety Checks: The function verifies whether the recipient is a contract and checks if it correctly handles ERC721 tokens. If the recipient does not meet the criteria, the transaction will revert, preventing the token from being sent to an unsupported contract. Reduced Attack Surface: Although _safeMint does not inherently protect against all forms of reentrancy, it minimizes the risk by ensuring tokens are only sent to contracts that can manage them appropriately.

[BogoCvetkov]

The use of _mint here is intentional. The readme of this CTF explicitly states:

Assume depositors are the ones responsible for taking care that they can handle NFT's.

But your assumption is correct, it can score a Low in competitions.