Details & Impact
The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:
1)OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
2)Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()
Recommended Mitigation Steps
Call the safeTransferFrom() method instead of transferFrom()
Lines of code
https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/GobblerReserve.sol#L38 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L748
Vulnerability details
Vulnerability details
Details & Impact The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:
1)OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible 2)Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()
Recommended Mitigation Steps Call the safeTransferFrom() method instead of transferFrom()