Lines of code
<https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L378-L406&gt;

Vulnerability details

Impact

veNFTs may be sent to contracts that cannot handle them, and therefore all rewards and voting power, as well as the underlying are locked forever

Proof of Concept

The original code had the following warning:

    * @dev Safely transfers tokenId token from from to to, checking first that contract recipients
    * are aware of the ERC721 protocol to prevent tokens from being forever locked.

<https://github.com/solidlyexchange/solidly/blob/eac1ef02ca8736138b25062c02646ea43d5bb6e4/contracts/ve.sol#L143-L144&gt;

The minting code, which creates the locks, does not do this check:

File: contracts/contracts/VotingEscrow.sol   #1

462       function _mint(address _to, uint _tokenId) internal returns (bool) {
463           // Throws if _to is zero address
464           assert(_to != address(0));
465           // TODO add delegates
466           // checkpoint for gov
467           _moveTokenDelegates(address(0), delegates(_to), _tokenId);
468           // Add NFT. Throws if _tokenId is owned by someone
469           _addTokenTo(_to, _tokenId);
470           emit Transfer(address(0), _to, _tokenId);
471           return true;
472       }

<https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L462-L472&gt;

Once a lock is already minted, if it’s transfered to a contract, the code does attempt to check for the issue:

File: contracts/contracts/VotingEscrow.sol   #2

378       ///      If _to is a smart contract, it calls onERC721Received on _to and throws if
379       ///      the return value is not bytes4(keccak256("onERC721Received(address,address,uint,bytes)")).
380       /// @param _from The current owner of the NFT.
381       /// @param _to The new owner.
382       /// @param _tokenId The NFT to transfer.
383       /// @param _data Additional data with no specified format, sent in call to _to.
384       function safeTransferFrom(
385           address _from,
386           address _to,
387           uint _tokenId,
388           bytes memory _data
389       ) public {
390           _transferFrom(_from, _to, _tokenId, msg.sender);
391   
392           if (_isContract(_to)) {
393               // Throws if transfer destination is a contract which does not implement 'onERC721Received'
394               try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
395                   bytes memory reason
396               ) {
397                   if (reason.length == 0) {
398                       revert('ERC721: transfer to non ERC721Receiver implementer');
399                   } else {
400                       assembly {
401                           revert(add(32, reason), mload(reason))
402                       }
403                   }
404               }
405           }
406       }

<https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L378-L406&gt;

While the transfer function does in fact execute onERC721Received, it doesn’t actually do a check of the bytes4 variable - it only checks for a non-zero length. The ERC721 standard says that for the function call to be valid, it must return the bytes4 function selector, otherwise it’s invalid. If a user of the escrow system uses a contract that is attempting to explicitly reject NFTs by returning zero in its onERC721Received() function, the VotingEscrow will interpret that response as success and will transfer the NFT, potentially locking it forever. If the lock is minted to a contract, no checks are done, and the NFT can be locked forever.

Tools Used

Code inspection

Recommended Mitigation Steps

Call onERC721Received() in _mint() and ensure that the return value equals IERC721Receiver.onERC721Received.selector in both _mint() and safeTransferFrom()

---

The text was updated successfully, but these errors were encountered:

All reactions