Fjord Token Staking

Summary

The smart contract uses ERC721.transferFrom() instead of safeTransferFrom() when transferring NFTs. This approach lacks the built-in safety checks that safeTransferFrom() offers. Specifically, safeTransferFrom() ensures that if the recipient is a contract, it implements the `ERC721Receiver` interface, preventing NFTs from being sent to contracts that do not handle them correctly. Using `transferFrom()` can lead to situations where NFTs are lost or become irretrievable because they are sent to incompatible addresses.

Vulnerability Details

The vulnerability occurs because `transferFrom()` does not verify if the recipient is a smart contract capable of receiving and handling ERC721 tokens. While `safeTransferFrom()` includes an additional check to ensure that the receiving contract implements the `onERC721Received` function, `transferFrom()` does not. This oversight can result in NFTs being sent to contracts that do not support the ERC721 standard, potentially freezing or losing the tokens.

https://github.com/Cyfrin/2024-08-fjord/blob/main/src/FjordStaking.sol#L521-L564

When a user who staked using an NFT unstakes, the _unstakeVested function does a transfer as shown below.

If the streamOwner is a contract address that does not support ERC721, the NFT can be frozen in that contract.

As per the documentation of EIP-721:

> A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Impact

This will lead to loss of NFTs for the streamOwner especially if they use smart contracts like gnosis safe wallet to ,take which can recive fungible tokens without any issue but not for NFT.

Tools Used

Manual Review

Recommendations

Replace the use of `transferFrom()` with `safeTransferFrom()` in both the `_unstakeVested` and `stakeVested` functions. This ensures that NFTs can only be sent to addresses that are either externally owned accounts (EOAs) or contracts that correctly implement the `onERC721Received` function. By implementing this change, the risk of NFTs being lost in incompatible contracts is mitigated.