ZeroLend Report

gistfile1.txt

Findings

_transferFrom() does not adhere to the ERC721 standard as expected

Description

As outlined here in the NatSpec comments, it is expected that a call to _transferFrom() ”Throws if _to is the zero address”. However there is no check to ensure this is the case. Here is a test case that demonstrates this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import "../../contracts/ZeroLocker.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

// Deploy a mock ERC20 token for the set up
contract MockERC20 is ERC20 {
    constructor() ERC20("MockToken", "MOCK") {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }
}

contract TransferFromTest is Test, MockERC20 {
    MockERC20 public mockERC20;
    ZeroLocker public zeroLocker;
    TransparentUpgradeableProxy public transparentUpgradeableProxy;

    function setUp() public {
        // Deploy the contracts
        mockERC20 = new MockERC20();
        zeroLocker = new ZeroLocker();
        bytes memory initData = abi.encodeWithSelector(ZeroLocker.initialize.selector, address(mockERC20));
        transparentUpgradeableProxy = new TransparentUpgradeableProxy(address(zeroLocker), address(this), initData);
        zeroLocker = ZeroLocker(address(transparentUpgradeableProxy));
    }

    function test_Supply() public {
        // Create an address for Alice and mint her some ERC20 tokens for the lock
        address alice = makeAddr("alice");
        mockERC20.mint(alice, 1000e18);
        vm.startPrank(alice);
        mockERC20.approve(address(zeroLocker), 1000e18);

        // Alice creates a lock NFT
        uint256 tokenId = zeroLocker.createLock(1000e18, 1 weeks);

        // Transfer the NFT to the 0 address, expecting a revert
        vm.expectRevert();
        zeroLocker.transferFrom(alice, address(0), tokenId);
        vm.stopPrank();
    }
}

Here is the output:

[FAIL. Reason: call did not revert as expected] test_TransferFrom()

The call did not revert as expected and the token was successfully sent to the 0 address, making it unrecoverable**.** This affects both transferFrom() and safeTransferFrom() as _transfer() is called internally in both functions as seen here and here respectively**.** To prevent unexpected behaviour such as loss of tokens, it is critical to not deviate from the standard if it is expected that it should be followed as seen in this case.

Recommendation

Add the following check to _transferFrom(). ZeroLocker.sol#L334:

require(_to != address(0), "_to cannot be the 0 address");

With this change, if the above test case is ran again the function call does revert as expected. Here is the output:

[PASS] test_TransferFrom()