Noisy Chrome Beaver - Potential Bypass of Contract Check in safeTransferFrom Allows Transfers to Unverified Recipients

[sherlock-admin4]

Noisy Chrome Beaver

Low/Info

Potential Bypass of Contract Check in safeTransferFrom Allows Transfers to Unverified Recipients

Summary

The safeTransferFrom function in the VotingEscrow contract is vulnerable to a potential bypass of the _isContract() check, which could lead to tokens being transferred to a contract address that doesn't properly implement the ERC721Receiver interface, resulting in the tokens being stuck.

Vulnerability Detail

The safeTransferFrom function uses _isContract() to determine if the recipient is a contract. However, this check can be bypassed if the transfer is made to a contract address after its deployment, counting that contract has no runtime code and only creation code, the extcodesize check will return 0 making isContract() return false.

Impact

This vulnerability could lead to tokens being transferred to contract addresses that are not properly equipped to handle ERC721 tokens, potentially resulting in permanent loss of these tokens. The impact is significant as it could affect the core functionality of the token transfer mechanism. Since this is a function intended to be safe to call additional checks can be performed to avoid this to happend.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L411

Tool used

Manual Review

Recommendation

Add a check to ensure that the msg.sender is an EOA when transferring to what appears to be a non-contract address:

function safeTransferFrom(
    address _from,
    address _to,
    uint _tokenId,
    bytes memory _data
) public {
    _transferFrom(_from, _to, _tokenId, msg.sender);

    require(_to != address(0), "ERC721: transfer to the zero address");

    if (_isContract(_to)) {
        require(
            IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) == 
            IERC721Receiver(_to).onERC721Received.selector,
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    } else {
        require(msg.sender == tx.origin, "ERC721: caller must be EOA when recipient is not a contract");
    }
}