Use safetransferFrom instead of transferFrom for NFT(ERC721) transfers

Summary

The transferFrom() method is used instead of safeTransferFrom() in CollateralToken.sol file.

Vulnerability Detail

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible

Impact

Given that any NFT can be used here, 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()

Code Snippet

CollateralToken.sol#L176 CollateralToken.sol#L223

  176:     nft.transferFrom(address(this), address(receiver), tokenId);
  223:     IERC721(underlyingAsset).transferFrom(address(this), releaseTo, assetId);

Tool used

Manual Review

Recommendation

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers.

Duplicate of #293

sherlock-audit / 2022-10-astaria-judging

Sm4rty - Use safetransferFrom instead of transferFrom for NFT(ERC721) transfers #282