code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L462-L472 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L378-L406

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

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

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

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()

GalloDaSballo commented 2 years ago

While I would be surprised by any realistic scenario of a smart contract that would implement onERC721Received and not return the specified selector, I have to acknowledge that technically we can get multiple false positives, especially when dealing with contracts that have a fallback function .

That said, to me that was not sufficient to constitute a valid finding.

However, I went and checked the EIP-721 and the standard definition of safeTransferFrom, which can be verified here: https://eips.ethereum.org/EIPS/eip-721

According to the standard the function must check for the return value, and for that reason I believe the finding to be valid.

While I would be surprised if this causes any issue in the real world, because the function is non-conformant to the standard and technically a loss can happen because of it, I believe Medium Severity to be appropriate

GalloDaSballo commented 2 years ago

To confirm, I doubt any meaningful loss will ever happen.

However this finding is basically: "Incorrect implementation of onERC721Received" and it's a valid finding at that

code-423n4 / 2022-05-velodrome-findings