Unsafe miniting of position nfts

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc9744d2c5939517f8898f82f71d405f530cb3b42aecadba999ceb9ff71671507 Severity: medium

Description: Description\

Use safeMint instead of mint for ERC721 tokens.

Attack Scenario\ When a deposit is made into the staking service by a user, an nft position is minted to the caller. The nft uses unsafe _mint function which doesn't check that the receiver accepts ERC721 tokens. This can lead to the user's staking position being locked in the contract.

As per the documentation of ERC721Upgradeable.sol by Openzeppelin, the use of _mint is discouraged in favour of safeMint instead.

Ref:

/**

• @dev Mints tokenId and transfers it to to.
• WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
• Requirements:
• • tokenId must not exist.
• • to cannot be the zero address.
• Emits a {Transfer} event. */ function _mint(address to, uint256 tokenId) internal virtual {

Attachments

1. Proof of Concept (PoC) File

   
   
   function _deposit(
       uint256 tokenId,
       uint256 cvgCvxAmount,
       DepositCvxData memory cvxData,
       bool isEthDeposit
   ) internal {
   ...
       else {
           _tokenId = _cvxStakingPositionManager.mint(msg.sender);
       }

...

```solidity
    function mint(address account) external returns (uint256) {
        /// @dev Verify that caller is a StakingContract
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        /// @dev Increments the nextId
        uint256 tokenId = nextId++;

        /// @dev Link the minted token with the StakingService
        stakingPerTokenId[tokenId] = msg.sender;
        /// @dev Mints the token
        _mint(account, tokenId); //unsafe mint

        return tokenId;
    }

2. Revised Code File (Optional)

Use safemint function instead and implement a reentrancy guard

    function mint(address account) nonReentrant external returns (uint256) {
        /// @dev Verify that caller is a StakingContract
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        /// @dev Increments the nextId
        uint256 tokenId = nextId++;

        /// @dev Link the minted token with the StakingService
        stakingPerTokenId[tokenId] = msg.sender;
        /// @dev Mints the token
        _safeMint(account, tokenId);

        return tokenId;
    }

[rodiontr]

@PlamenTSV invalid issue as this is more of a recommendation than a vulnerability. _safeMint() in its turn opens up reentrancy attack vector so both functions have their advantages and disadvantages

[0xMR0]

@PlamenTSV i think @rodiontr is correct here. A normal mint function can also send the NFT to both wallet and contract address. You always dont need to check ERC721 receiver support unless the safe transfer method is used to transfer NFT. You can check it in remix IDE. use of unsafe mint seems to be intended design by project team.

[0xR3vert]

We don't use safeMint, as it's an expensive function and can be exploited by reentrancy. This issue is therefore invalid.