Council Member may lose tokens if not meant to receive ERC721 tokens.

Summary

The CouncilMember::removeFromOffice() function replaces an existing council member with a new one and withdraws the former member's TELCOIN allocation. Although the function includes the transfer of an NFT, it lacks certain necessary checks.

Vulnerability Detail

Following the distribution and withdrawal of tokens to the recipient, the function proceeds to transfer the "Membership" NFT token from one address to another. This transfer is executed using the ERC721Upgradeable.sol::_transfer() function. It's important to note that OpenZeppelin’s documentation advises against using transferFrom() and recommends using safeTransferFrom() whenever possible. The vulnerability lies in the fact that the function uses transferFrom() instead of the safer alternative.

The potential risk arises from the fact that the recipient might have specific logic within the onERC721Received() function, which is exclusively triggered in the safeTransferFrom() function, but not in transferFrom().

Impact

While the likelihood of this vulnerability being exploited is low, as the recipient is typically the function caller, there exists a potential risk of losing NFTs if the recipient is unable to appropriately handle the ERC721 tokens sent to them.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L133

Tool used

Manual Review

Recommendation

Use safeTransferFrom() instead of transferFrom() for outgoing erc721 transfers

 function removeFromOffice(
     address from,
     address to,
     uint256 tokenId,
     address rewardRecipient
 ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
     // Retrieve and distribute any pending TELCOIN for all council members
     _retrieve();
     // Withdraw all the TELCOIN rewards for the specified token to the rewardRecipient
     _withdrawAll(rewardRecipient, tokenId);
-    // Transfer the token (representing the council membership) from one address to another
-    _transfer(from, to, tokenId);
+    // Safe transfer the token (representing the council membership) from one address to another
+    _safeTransfer(from, to, tokenId);
 }

Duplicate of #25

sherlock-audit / 2024-01-telcoin-judging

eeshenggoh - Council Member may lose tokens if not meant to receive ERC721 tokens. #117