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

Aperta
  • Qualcosa non va?
#300 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
2/5
Tempo stimato
1-3 ore
Idoneità per principianti
55/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
solidity
Ambito
blockchain

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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

Lingua principale
Solidity
Stelle
1k
Fork
391
Nessuna PR unita negli ultimi 30g

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di fulldecent/erc721

Tutte le issue di fulldecent/erc721

Issue simili

Altre issue su Blockchain

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.