From e2e643cf5f8167fdf0ae66c50152b6711e061359 Mon Sep 17 00:00:00 2001 From: alexfertel Date: Thu, 1 Aug 2024 16:53:41 +0200 Subject: [PATCH] fix(erc721): properly handle receiver errors (#225) Resolves #224 The `FIXME`s added in the PR are tracked [here](https://github.com/OpenZeppelin/rust-contracts-stylus/issues/228). - [x] Tests --- contracts/src/token/erc721/mod.rs | 66 +++++++++++-------- examples/erc721/tests/erc721.rs | 101 ++++++++++++++++++++++++++++-- 2 files changed, 136 insertions(+), 31 deletions(-) diff --git a/contracts/src/token/erc721/mod.rs b/contracts/src/token/erc721/mod.rs index a3b97b0d..1abe8600 100644 --- a/contracts/src/token/erc721/mod.rs +++ b/contracts/src/token/erc721/mod.rs @@ -3,7 +3,11 @@ use alloc::vec; use alloy_primitives::{fixed_bytes, uint, Address, FixedBytes, U128, U256}; use stylus_sdk::{ - abi::Bytes, alloy_sol_types::sol, call::Call, evm, msg, prelude::*, + abi::Bytes, + alloy_sol_types::sol, + call::{self, Call}, + evm, msg, + prelude::*, }; use crate::utils::math::storage::{AddAssignUnchecked, SubAssignUnchecked}; @@ -133,6 +137,9 @@ pub enum Error { InvalidSender(ERC721InvalidSender), /// Indicates a failure with the token `receiver`. Used in transfers. InvalidReceiver(ERC721InvalidReceiver), + /// Indicates a failure with the token `receiver`, with the reason + /// specified by it. + InvalidReceiverWithReason(call::Error), /// Indicates a failure with the `operator`’s approval. Used in transfers. InsufficientApproval(ERC721InsufficientApproval), /// Indicates a failure with the `approver` of a token to be approved. Used @@ -1051,11 +1058,10 @@ impl Erc721 { /// `operator` is generally the address that initiated the token transfer /// (i.e. `msg::sender()`). /// - /// The acceptance call is not executed and treated as a no-op - /// if the target address doesn't contain code (i.e. an EOA). - /// Otherwise, the recipient must implement - /// [`IERC721Receiver::on_erc_721_received`] and return the - /// acceptance magic value to accept the transfer. + /// The acceptance call is not executed and treated as a no-op if the + /// target address doesn't contain code (i.e. an EOA). Otherwise, the + /// recipient must implement [`IERC721Receiver::on_erc_721_received`] and + /// return the acceptance magic value to accept the transfer. /// /// # Arguments /// @@ -1080,29 +1086,37 @@ impl Erc721 { token_id: U256, data: &Bytes, ) -> Result<(), Error> { - const IERC721RECEIVER_INTERFACE_ID: FixedBytes<4> = - fixed_bytes!("150b7a02"); - - // FIXME: Cleanup this code once it's covered in the test suite. - if to.has_code() { - let call = Call::new_in(self); - return match IERC721Receiver::new(to).on_erc_721_received( - call, - operator, - from, - token_id, - data.to_vec(), - ) { - Ok(result) => { - if result == IERC721RECEIVER_INTERFACE_ID { - Ok(()) - } else { - Err(ERC721InvalidReceiver { receiver: to }.into()) + const RECEIVER_FN_SELECTOR: FixedBytes<4> = fixed_bytes!("150b7a02"); + + if !to.has_code() { + return Ok(()); + } + + let receiver = IERC721Receiver::new(to); + let call = Call::new_in(self); + let data = data.to_vec(); + let result = + receiver.on_erc_721_received(call, operator, from, token_id, data); + + let id = match result { + Ok(id) => id, + Err(e) => { + if let call::Error::Revert(ref reason) = e { + if reason.len() > 0 { + // Non-IERC721Receiver implementer. + return Err(Error::InvalidReceiverWithReason(e)); } } - Err(_) => Err(ERC721InvalidReceiver { receiver: to }.into()), - }; + + return Err(ERC721InvalidReceiver { receiver: to }.into()); + } + }; + + // Token rejected. + if id != RECEIVER_FN_SELECTOR { + return Err(ERC721InvalidReceiver { receiver: to }.into()); } + Ok(()) } } diff --git a/examples/erc721/tests/erc721.rs b/examples/erc721/tests/erc721.rs index 64ec3924..b3cb264a 100644 --- a/examples/erc721/tests/erc721.rs +++ b/examples/erc721/tests/erc721.rs @@ -737,9 +737,6 @@ async fn error_when_safe_transfer_nonexistent_token( Ok(()) } -// TODO: Test reverts & panics on ERC721ReceiverMock for -// `Erc721::safeTransferFrom_0`. - #[e2e::test] async fn safe_transfers_from_with_data( alice: Account, @@ -1092,8 +1089,102 @@ async fn error_when_safe_transfer_with_data_nonexistent_token( Ok(()) } -// TODO: Test reverts & panics on ERC721ReceiverMock for -// `Erc721::safeTransferFrom_1`. +// FIXME: Update our `reverted_with` implementation such that we can also check +// when the error is a `stylus_sdk::call::Error`. +#[e2e::test] +#[ignore] +async fn errors_when_receiver_reverts_with_reason( + alice: Account, +) -> eyre::Result<()> { + let contract_addr = deploy(alice.url(), &alice.pk()).await?; + let contract = Erc721::new(contract_addr, &alice.wallet); + + let receiver_address = receiver::deploy( + &alice.wallet, + ERC721ReceiverMock::RevertType::RevertWithMessage, + ) + .await?; + + let alice_addr = alice.address(); + let token_id = random_token_id(); + + let _ = watch!(contract.mint(alice_addr, token_id))?; + + let _err = send!(contract.safeTransferFrom_0( + alice_addr, + receiver_address, + token_id + )) + .expect_err("should not transfer when receiver errors with reason"); + + // assert!(err.reverted_with(stylus_sdk::call::Error::Revert( + // b"ERC721ReceiverMock: reverting".to_vec() + // ))); + Ok(()) +} + +#[e2e::test] +async fn errors_when_receiver_reverts_without_reason( + alice: Account, +) -> eyre::Result<()> { + let contract_addr = deploy(alice.url(), &alice.pk()).await?; + let contract = Erc721::new(contract_addr, &alice.wallet); + + let receiver_address = receiver::deploy( + &alice.wallet, + ERC721ReceiverMock::RevertType::RevertWithoutMessage, + ) + .await?; + + let alice_addr = alice.address(); + let token_id = random_token_id(); + + let _ = watch!(contract.mint(alice_addr, token_id))?; + + let err = send!(contract.safeTransferFrom_0( + alice_addr, + receiver_address, + token_id + )) + .expect_err("should not transfer when receiver reverts"); + + assert!(err.reverted_with(Erc721::ERC721InvalidReceiver { + receiver: receiver_address + })); + + Ok(()) +} + +// FIXME: Update our `reverted_with` implementation such that we can also check +// when the error is a `stylus_sdk::call::Error`. +#[e2e::test] +#[ignore] +async fn errors_when_receiver_panics(alice: Account) -> eyre::Result<()> { + let contract_addr = deploy(alice.url(), &alice.pk()).await?; + let contract = Erc721::new(contract_addr, &alice.wallet); + + let receiver_address = + receiver::deploy(&alice.wallet, ERC721ReceiverMock::RevertType::Panic) + .await?; + + let alice_addr = alice.address(); + let token_id = random_token_id(); + + let _ = watch!(contract.mint(alice_addr, token_id))?; + + let err = send!(contract.safeTransferFrom_0( + alice_addr, + receiver_address, + token_id + )) + .expect_err("should not transfer when receiver panics"); + + assert!(err.reverted_with(Erc721::ERC721InvalidReceiver { + receiver: receiver_address + })); + + Ok(()) +} #[e2e::test] async fn approves(alice: Account, bob: Account) -> eyre::Result<()> {