JPEG'd contest - Kenshin's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/helpers/CryptoPunksHelper.sol#L38 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/helpers/CryptoPunksHelper.sol#L52

Vulnerability details

Impact

In the NFT helper contract, there is no validation on that the receiver address must not be address zero. Therefore, it allows owner or an attacker who gain access to the owner address to burn NFTs forever through the functions by transferring the NFTs to address zero.

Proof of Concept

The PoC is originally conducted using foundry. However, It isn't that complicated so I rewrote it in TypeScipt as well, the team can easily proof this by including in the CryptoPunksHelper.ts.

TypeScript

// add `.only` to run only this test, not all.
it.only("allows the owner to burn nfts", async () => {
    // safeTransferFrom
    await cryptoPunks.getPunk(1);
    await cryptoPunks.transferPunk(helper.address, 1);
    await helper.safeTransferFrom(owner.address, ZERO_ADDRESS, 1);
    expect(await cryptoPunks.punkIndexToAddress(1)).to.equal(ZERO_ADDRESS);
    expect(await helper.ownerOf(1)).to.equal(ZERO_ADDRESS);

    // transferFrom
    await cryptoPunks.getPunk(2);
    await cryptoPunks.transferPunk(helper.address, 2);
    await helper.transferFrom(owner.address, ZERO_ADDRESS, 2);
    expect(await cryptoPunks.punkIndexToAddress(2)).to.equal(ZERO_ADDRESS);
    expect(await helper.ownerOf(2)).to.equal(ZERO_ADDRESS);
  });

Foundry

pragma solidity ^0.8.0;

// for test
import "ds-test/test.sol";
import "forge-std/Vm.sol";

// contracts
import "../test/CryptoPunks.sol";
import "../helpers/CryptoPunksHelper.sol";

contract CryptoPunksHelperTest is DSTest {
    Vm constant vm = Vm(HEVM_ADDRESS);
    
    address owner = address(1);
    address user = address(2);
    
    CryptoPunks private cps;
    CryptoPunksHelper private helper;

    function setUp() public {
        vm.startPrank(owner);
        cps = new CryptoPunks();
        helper = new CryptoPunksHelper();
        helper.initialize(address(cps));
        vm.stopPrank();
    }

    function testOwnerTransferToZero() public {
        //make sure address zero hold no punks
        assertEq(cps.balanceOf(address(0)), 0);

        // safeTransferFrom PoC
        vm.startPrank(owner);
        cps.getPunk(1);
        cps.transferPunk(address(helper), 1);
        helper.safeTransferFrom(owner, address(0), 1);
        assertEq(cps.punkIndexToAddress(1), address(0));
        assertEq(helper.ownerOf(1), address(0));
        assertEq(cps.balanceOf(address(0)), 1);

        // transferFrom PoC
        cps.getPunk(2);
        cps.transferPunk(address(helper), 2);
        helper.transferFrom(owner, address(0), 2);
        assertEq(cps.punkIndexToAddress(2), address(0));
        assertEq(helper.ownerOf(2), address(0));
        assertEq(cps.balanceOf(address(0)), 2);
    }
}

foundry.toml

[default]
src = "contracts"
libs = ["lib/forge-std/lib", "lib/", "node_modules"]
solc_version = "0.8.0"
optimizer = false
fuzz_runs = 100000
test = "foundryTest"

Tools Used

• Foundry
• Hardhat

Recommended Mitigation Steps

Even the functions are restricted for only the owner, the zero address should not be allowed as the receiver address.