Bug Bounty Report: phantom token IDs revert with authorization codes (003003/003004) instead of NOT_VALID_NFT (003002) on transferFrom/approve/safeTransferFrom

Abierto
  • ¿Algo no está bien?
#300 2 comentarios 0 reacciones 0 asignados Ver en GitHub

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
2/5
Tiempo estimado
1-3 horas
Aptitud para principiantes
55/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
solidity
Área
blockchain

Línea de trabajo

Start in src/contracts/tokens/nf-token.sol: compare the modifier order on transferFrom (around lines 174-175), approve (197-198) and _safeTransferFrom (431-432), then read canTransfer and canOperate (64-88) and validNFToken (99). Reproduce with the Foundry test in the report, which calls ownerOf, transferFrom, approve and safeTransferFrom on phantom ID 999. Done means all entry points return 003002 for a nonexistent token, or the natspec documents the per-entry-point codes. The maintainer must choose between those two, so confirm the direction in the thread before editing.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

Found while reviewing src/contracts under the bug bounty program (latest release, @nibbstack/erc721 2.6.1).

What

transferFrom, approve, and _safeTransferFrom declare their authorization modifier before validNFToken, so for a token ID that does not exist the transaction reverts with an authorization-flavored code instead of NOT_VALID_NFT:

src/contracts/tokens/nf-token.sol:174-175 (transferFrom):

    canTransfer(_tokenId)
    validNFToken(_tokenId)

src/contracts/tokens/nf-token.sol:197-198 (approve):

    canOperate(_tokenId)
    validNFToken(_tokenId)

src/contracts/tokens/nf-token.sol:431-432 (_safeTransferFrom): same order.

Modifiers run in declaration order, and both canTransfer and canOperate read idToOwner[_tokenId] directly:

src/contracts/tokens/nf-token.sol:64-69 and :82-88:

address tokenOwner = idToOwner[_tokenId];
require(
  tokenOwner == msg.sender || ownerToOperators[tokenOwner][msg.sender],
  NOT_OWNER_OR_OPERATOR        // "003003"
);

For a phantom ID tokenOwner is address(0), which never equals msg.sender, so the require fails with 003003 (approve) or 003004 (transferFrom) and validNFToken (nf-token.sol:99, NOT_VALID_NFT = "003002") is never reached.

Meanwhile the same natspec promises the validity check: src/contracts/tokens/nf-token.sol:160-161 says "Throws if _tokenId is not a valid NFT", and ownerOf (:258) plus getApproved (:273) do return 003002 for the same phantom ID.

Impact

The revert code for "this token does not exist" depends on which entry point the caller hit: 003002 from ownerOf/getApproved, but 003003/003004 from transferFrom/approve/safeTransferFrom. Indexers, front ends, and integrators that branch on 003002 to distinguish "nonexistent token" from "not authorized" misclassify every nonexistent-token call that arrives through a transfer or approve entry point, and the documented behavior does not mention this. This falls under "any undocumented and unintuitive behavior" in the program scope.

Suggested fix

List validNFToken(_tokenId) before canTransfer/canOperate on all three entry points (Solidity checks modifiers in order), or document the actual codes per entry point in the natspec.

Proof (Foundry, solc 0.8.26, forge-std)

[PASS] test_approve_phantom_returns_003003_not_003002() (gas: 17523)
[PASS] test_ownerOf_phantom_returns_003002() (gas: 12216)
[PASS] test_safeTransferFrom_phantom_returns_003004_not_003002() (gas: 20329)
[PASS] test_transferFrom_phantom_returns_003004_not_003002() (gas: 20225)
Suite result: ok. 4 passed; 0 failed; 0 skipped

Test contract:

contract RevertCodeMock is NFToken, Ownable {
  function mint(address _to, uint256 _tokenId) external onlyOwner { super._mint(_to, _tokenId); }
}
// setUp: nft.mint(alice, 1); phantom id = 999
vm.expectRevert(bytes("003002")); nft.ownerOf(999);
vm.prank(alice); vm.expectRevert(bytes("003004")); nft.transferFrom(alice, alice, 999);
vm.prank(alice); vm.expectRevert(bytes("003003")); nft.approve(alice, 999);
vm.prank(alice); vm.expectRevert(bytes("003004")); nft.safeTransferFrom(alice, alice, 999);

Tested against tag 2.6.1 (commit 198dec9).

Lenguaje dominante
Solidity
Estrellas
1k
Forks
391
Sin PR fusionados en 30 d

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de fulldecent/erc721

Todos los issues de fulldecent/erc721

Issues similares

Más issues de Blockchain

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.