BondNFT.sol and GovNFT.sol: safeTransferMany should call a safe function

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L282
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L245

Vulnerability details

Impact

The BondNFT and GovNFT contracts provide a safeTransferMany function:

BondNFT.safeTransferMany: https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/BondNFT.sol#L282

GovNFT.safeTransferMany: https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L245

The issue is that both of these functions are not actually "safe".

"safe" in the context of an ERC721 means that if the receiver of a token transfer is a contract, it must implement the onERC721Received function.

The contract signals by implementing it that it can handle ERC721 transfers.

So in this case it is misleading that the functions are labelled "safe" but are not actually "safe".

Especially since BondNFT.safeTransferFromMany and GovNFT.safeTransferFromMany are indeed "safe" since they call safeTransferFrom internally whereas the safeTransferMany functions call _transfer internally. So I believe this is just an oversight that occurred while coding and the safeTransferMany functions should call a "safe" function internally as well.

Since it looks like the safeTransferMany functions are safe when indeed they are not, tokens can be transferred to a contract that cannot handle ERC721s, thereby the tokens are lost.

Proof of Concept

1. User A owns two BondNFT tokens
2. User A wants to send the tokens to a contract which he assumes can handle ERC721s but is not sure. By calling BondNFT.safeTransferMany, he thinks it is first checked whether the contract implements onERC721Received since "safe" has this meaning for ERC721 transfers.
3. The contract does not implement onERC721Received and cannot handle ERC721s. The tokens are lost.

Tools Used

VSCode

Recommended Mitigation Steps

In BondNFT.safeTransferMany and GovNFT.safeTransferMany, change the line

_transfer(_msgSender(), _to, _ids[i]);

to

safeTransferFrom(_msgSender(), _to, _ids[i]);  

[c4-judge]

GalloDaSballo marked the issue as duplicate of #356

[c4-judge]

GalloDaSballo marked the issue as satisfactory