Skip to content

Enforce Checks-Effects-Interactions pattern in NFTSwap purchase and revoke#896

Open
daatsuka wants to merge 1 commit intoAmazingAng:mainfrom
daatsuka:correct-nft-claim-completion
Open

Enforce Checks-Effects-Interactions pattern in NFTSwap purchase and revoke#896
daatsuka wants to merge 1 commit intoAmazingAng:mainfrom
daatsuka:correct-nft-claim-completion

Conversation

@daatsuka
Copy link
Copy Markdown

What type of PR is this (这是什么类型的PR)

  • Typo(勘误)
  • BUG(程序错误)
  • Improvement(提升)
  • Feature(新特性)

Which issue(s) this PR fixes(Optional) (这个PR 修复了什么问题 (可选择))

What this PR does / why we need it (这个PR 做了什么/ 我们为什么需要这个PR)

The purchase() and revoke() functions in 38_NFTSwap/NFTSwap.sol (and its English counterpart at Languages/en/38_NFTSwap_en/NFTSwap.sol) were deleting the nftList mapping entry after performing external calls — safeTransferFrom and transfer. This ordering violates the Checks-Effects-Interactions (CEI) pattern and opens the door to reentrancy: a malicious onERC721Received callback could re-enter purchase() before the order is cleared, draining the contract. The recompiled out/ artifacts reflect the updated bytecode produced by the Solidity 0.8.34 toolchain that was bumped in the prior commit on main.

In purchase(), I now cache _order.owner and _order.price into local memory variables, then delete nftList[_nftAddr][_tokenId] immediately after the require checks and before any external interaction. The subsequent safeTransferFrom, transfer, and refund logic all reference the cached locals instead of reading from storage that no longer exists. The same state-before-interaction reordering is applied to revoke(), where the delete now precedes the safeTransferFrom call that returns the NFT to the seller. Both the Chinese and English source files carry identical logic changes so the tutorial stays consistent across languages.

The design intent is straightforward: state mutations must settle before the contract hands control to an external address. This is the canonical CEI discipline recommended by the Solidity docs and auditing firms alike, and it eliminates the reentrancy vector without introducing a mutex lock or any additional gas overhead. I verified the change locally by compiling with forge build — all contracts compile cleanly with zero warnings, and the generated artifacts in out/ match the expected output for the updated source.

Closes #895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Unable to claim the NFT upon course completion

1 participant