revert: "feat: Turn All Call::new_in into Call::new (i.e. Stop Su…

…pporting Reentrancy) (#440)" (#464) Towards #355 Reentrancy is necessary for Erc20FlashMint. refs: da0249b
OpenZeppelin · Dec 18, 2024 · 4de08e5 · 4de08e5
1 parent 5fce6db
commit 4de08e5
Show file tree

Hide file tree

Showing 12 changed files with 68 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [v0.2.0-alpha.2] - 2024-12-17
+## [v0.2.0-alpha.2] - 2024-12-18
 
 ### Added
 
@@ -19,20 +19,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- Update "magic values" to explicit calculations in `Erc721Metadata::supports_interface`, and
-  `Erc721::_check_on_erc721_received`. #442
+- Update "magic values" to explicit calculations in `Erc721Metadata::supports_interface`, and `Erc721::_check_on_erc721_received`. #442
 - Implement `AddAssignUnchecked` and `SubAssignUnchecked` for `StorageUint`. #418
 - Implement `MethodError` for `safe_erc20::Error`. #402
 - Use `function_selector!` to calculate transfer type selector in `Erc1155`. #417
 
 ### Changed (Breaking)
 
 - Update internal functions of `Erc721` and `Erc721Consecutive` to accept a reference to `Bytes`. #437
-- Stop supporting reentrancy, and borrow `self` immutably in `IErc721::_check_on_erc721_received`. #440
-- Remove `&mut self` parameter from `IErc1155::_check_on_erc1155_received` and make it an associated function. #440
-- Remove `storage: &mut impl TopLevelStorage` parameter from `ecdsa::recover`. #440
-- Remove `TopLevelStorage` trait implementation from `VestingWallet`, `Erc1155`, `Erc20Permit`, `SafeErc20`,
-  `Erc721Consecutive`, and `Erc721`. #440
 
 ### Fixed
 

diff --git a/contracts/src/finance/vesting_wallet.rs b/contracts/src/finance/vesting_wallet.rs
@@ -33,7 +33,7 @@ use stylus_sdk::{
     call::{self, call, Call},
     contract, evm, function_selector,
     prelude::storage,
-    storage::{StorageMap, StorageU256, StorageU64},
+    storage::{StorageMap, StorageU256, StorageU64, TopLevelStorage},
     stylus_proc::{public, SolidityError},
 };
 
@@ -118,6 +118,11 @@ pub struct VestingWallet {
     pub safe_erc20: SafeErc20,
 }
 
+/// NOTE: Implementation of [`TopLevelStorage`] to be able use `&mut self` when
+/// calling other contracts and not `&mut (impl TopLevelStorage +
+/// BorrowMut<Self>)`. Should be fixed in the future by the Stylus team.
+unsafe impl TopLevelStorage for VestingWallet {}
+
 /// Required interface of a [`VestingWallet`] compliant contract.
 #[interface_id]
 pub trait IVestingWallet {
@@ -421,7 +426,7 @@ impl IVestingWallet for VestingWallet {
 
         let owner = self.ownable.owner();
 
-        call(Call::new().value(amount), owner, &[])?;
+        call(Call::new_in(self).value(amount), owner, &[])?;
 
         evm::log(EtherReleased { amount });
 

diff --git a/contracts/src/token/erc1155/extensions/supply.rs b/contracts/src/token/erc1155/extensions/supply.rs
@@ -279,7 +279,7 @@ impl Erc1155Supply {
         self._update(from, to, ids.clone(), values.clone())?;
 
         if !to.is_zero() {
-            Erc1155::_check_on_erc1155_received(
+            self.erc1155._check_on_erc1155_received(
                 msg::sender(),
                 from,
                 to,

diff --git a/contracts/src/token/erc1155/extensions/uri_storage.rs b/contracts/src/token/erc1155/extensions/uri_storage.rs
@@ -94,7 +94,7 @@ mod tests {
     use stylus_sdk::prelude::storage;
 
     use super::Erc1155UriStorage;
-    use crate::token::erc1155::extensions::Erc1155MetadataUri;
+    use crate::token::erc1155::{extensions::Erc1155MetadataUri, Erc1155};
 
     fn random_token_id() -> U256 {
         let num: u32 = rand::random();

diff --git a/contracts/src/token/erc1155/mod.rs b/contracts/src/token/erc1155/mod.rs
@@ -8,7 +8,7 @@ use stylus_sdk::{
     call::{self, Call, MethodError},
     evm, function_selector, msg,
     prelude::{public, storage, AddressVM, SolidityError},
-    storage::{StorageBool, StorageMap, StorageU256},
+    storage::{StorageBool, StorageMap, StorageU256, TopLevelStorage},
 };
 
 use crate::utils::{
@@ -194,6 +194,11 @@ pub struct Erc1155 {
         StorageMap<Address, StorageMap<Address, StorageBool>>,
 }
 
+/// NOTE: Implementation of [`TopLevelStorage`] to be able use `&mut self` when
+/// calling other contracts and not `&mut (impl TopLevelStorage +
+/// BorrowMut<Self>)`. Should be fixed in the future by the Stylus team.
+unsafe impl TopLevelStorage for Erc1155 {}
+
 /// Required interface of an [`Erc1155`] compliant contract.
 #[interface_id]
 pub trait IErc1155 {
@@ -557,7 +562,7 @@ impl Erc1155 {
         self._update(from, to, ids.clone(), values.clone())?;
 
         if !to.is_zero() {
-            Erc1155::_check_on_erc1155_received(
+            self._check_on_erc1155_received(
                 msg::sender(),
                 from,
                 to,
@@ -767,6 +772,7 @@ impl Erc1155 {
     ///
     /// # Arguments
     ///
+    /// * `&mut self` - Write access to the contract's state.
     /// * `operator` - Generally the address that initiated the token transfer
     ///   (e.g. `msg::sender()`).
     /// * `from` - Account of the sender.
@@ -785,6 +791,7 @@ impl Erc1155 {
     /// interface id or returned with error, then the error
     /// [`Error::InvalidReceiver`] is returned.
     fn _check_on_erc1155_received(
+        &mut self,
         operator: Address,
         from: Address,
         to: Address,
@@ -796,7 +803,7 @@ impl Erc1155 {
         }
 
         let receiver = IERC1155Receiver::new(to);
-        let call = Call::new();
+        let call = Call::new_in(self);
         let result = match details.transfer {
             Transfer::Single { id, value } => receiver
                 .on_erc_1155_received(call, operator, from, id, value, data),

diff --git a/contracts/src/token/erc20/extensions/permit.rs b/contracts/src/token/erc20/extensions/permit.rs
@@ -17,6 +17,7 @@ use alloy_sol_types::SolType;
 use stylus_sdk::{
     block,
     prelude::{storage, StorageType},
+    storage::TopLevelStorage,
     stylus_proc::{public, SolidityError},
 };
 
@@ -81,6 +82,11 @@ pub struct Erc20Permit<T: IEip712 + StorageType> {
     pub eip712: T,
 }
 
+/// NOTE: Implementation of [`TopLevelStorage`] to be able use `&mut self` when
+/// calling other contracts and not `&mut (impl TopLevelStorage +
+/// BorrowMut<Self>)`. Should be fixed in the future by the Stylus team.
+unsafe impl<T: IEip712 + StorageType> TopLevelStorage for Erc20Permit<T> {}
+
 #[public]
 impl<T: IEip712 + StorageType> Erc20Permit<T> {
     /// Returns the current nonce for `owner`.
@@ -171,7 +177,7 @@ impl<T: IEip712 + StorageType> Erc20Permit<T> {
 
         let hash: B256 = self.eip712.hash_typed_data_v4(struct_hash);
 
-        let signer: Address = ecdsa::recover(hash, v, r, s)?;
+        let signer: Address = ecdsa::recover(self, hash, v, r, s)?;
 
         if signer != owner {
             return Err(ERC2612InvalidSigner { signer, owner }.into());

diff --git a/contracts/src/token/erc20/utils/safe_erc20.rs b/contracts/src/token/erc20/utils/safe_erc20.rs
@@ -18,6 +18,7 @@ use stylus_sdk::{
     evm::gas_left,
     function_selector,
     prelude::storage,
+    storage::TopLevelStorage,
     stylus_proc::{public, SolidityError},
     types::AddressVM,
 };
@@ -87,6 +88,11 @@ mod token {
 #[storage]
 pub struct SafeErc20 {}
 
+/// NOTE: Implementation of [`TopLevelStorage`] to be able use `&mut self` when
+/// calling other contracts and not `&mut (impl TopLevelStorage +
+/// BorrowMut<Self>)`. Should be fixed in the future by the Stylus team.
+unsafe impl TopLevelStorage for SafeErc20 {}
+
 /// Required interface of a [`SafeErc20`] utility contract.
 pub trait ISafeErc20 {
     /// The error type associated to this trait implementation.

diff --git a/contracts/src/token/erc721/extensions/consecutive.rs b/contracts/src/token/erc721/extensions/consecutive.rs
@@ -30,7 +30,7 @@ use alloy_primitives::{uint, Address, U256};
 use stylus_sdk::{
     abi::Bytes,
     evm, msg,
-    prelude::storage,
+    prelude::{storage, TopLevelStorage},
     stylus_proc::{public, SolidityError},
 };
 
@@ -140,6 +140,8 @@ pub enum Error {
     ForbiddenBatchBurn(ERC721ForbiddenBatchBurn),
 }
 
+unsafe impl TopLevelStorage for Erc721Consecutive {}
+
 // ************** ERC-721 External **************
 
 #[public]

diff --git a/contracts/src/token/erc721/mod.rs b/contracts/src/token/erc721/mod.rs
@@ -207,6 +207,11 @@ pub struct Erc721 {
         StorageMap<Address, StorageMap<Address, StorageBool>>,
 }
 
+/// NOTE: Implementation of [`TopLevelStorage`] to be able use `&mut self` when
+/// calling other contracts and not `&mut (impl TopLevelStorage +
+/// BorrowMut<Self>)`. Should be fixed in the future by the Stylus team.
+unsafe impl TopLevelStorage for Erc721 {}
+
 /// Required interface of an [`Erc721`] compliant contract.
 #[interface_id]
 pub trait IErc721 {
@@ -1099,7 +1104,7 @@ impl Erc721 {
     ///
     /// # Arguments
     ///
-    /// * `&self` - Read access to the contract's state.
+    /// * `&mut self` - Write access to the contract's state.
     /// * `operator` - Account to add to the set of authorized operators.
     /// * `from` - Account of the sender.
     /// * `to` - Account of the recipient.
@@ -1113,7 +1118,7 @@ impl Erc721 {
     /// interface id or returned with error, then the error
     /// [`Error::InvalidReceiver`] is returned.
     pub fn _check_on_erc721_received(
-        &self,
+        &mut self,
         operator: Address,
         from: Address,
         to: Address,
@@ -1125,7 +1130,7 @@ impl Erc721 {
         }
 
         let receiver = IERC721Receiver::new(to);
-        let call = Call::new();
+        let call = Call::new_in(self);
         let result = receiver.on_erc_721_received(
             call,
             operator,

diff --git a/contracts/src/utils/cryptography/ecdsa.rs b/contracts/src/utils/cryptography/ecdsa.rs
@@ -8,6 +8,7 @@ use alloy_primitives::{address, uint, Address, B256, U256};
 use alloy_sol_types::SolType;
 use stylus_sdk::{
     call::{self, Call, MethodError},
+    storage::TopLevelStorage,
     stylus_proc::SolidityError,
 };
 
@@ -76,6 +77,7 @@ impl MethodError for ecdsa::Error {
 ///
 /// # Arguments
 ///
+/// * `storage` - Write access to storage.
 /// * `hash` - Hash of the message.
 /// * `v` - `v` value from the signature.
 /// * `r` - `r` value from the signature.
@@ -91,10 +93,16 @@ impl MethodError for ecdsa::Error {
 /// # Panics
 ///
 /// * If the `ecrecover` precompile fails to execute.
-pub fn recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
+pub fn recover(
+    storage: &mut impl TopLevelStorage,
+    hash: B256,
+    v: u8,
+    r: B256,
+    s: B256,
+) -> Result<Address, Error> {
     check_if_malleable(&s)?;
     // If the signature is valid (and not malleable), return the signer address.
-    _recover(hash, v, r, s)
+    _recover(storage, hash, v, r, s)
 }
 
 /// Calls `ecrecover` EVM precompile.
@@ -104,6 +112,7 @@ pub fn recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
 ///
 /// # Arguments
 ///
+/// * `storage` - Write access to storage.
 /// * `hash` - Hash of the message.
 /// * `v` - `v` value from the signature.
 /// * `r` - `r` value from the signature.
@@ -119,7 +128,13 @@ pub fn recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
 /// # Panics
 ///
 /// * If the `ecrecover` precompile fails to execute.
-fn _recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
+fn _recover(
+    storage: &mut impl TopLevelStorage,
+    hash: B256,
+    v: u8,
+    r: B256,
+    s: B256,
+) -> Result<Address, Error> {
     let calldata = encode_calldata(hash, v, r, s);
 
     if v == 0 || v == 1 {
@@ -130,8 +145,9 @@ fn _recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
         return Err(ECDSAInvalidSignature {}.into());
     }
 
-    let recovered = call::static_call(Call::new(), ECRECOVER_ADDR, &calldata)
-        .expect("should call `ecrecover` precompile");
+    let recovered =
+        call::static_call(Call::new_in(storage), ECRECOVER_ADDR, &calldata)
+            .expect("should call `ecrecover` precompile");
 
     let recovered = Address::from_slice(&recovered[12..]);
 

diff --git a/examples/ecdsa/src/lib.rs b/examples/ecdsa/src/lib.rs
@@ -20,7 +20,7 @@ impl ECDSAExample {
         r: B256,
         s: B256,
     ) -> Result<Address, Vec<u8>> {
-        let signer = ecdsa::recover(hash, v, r, s)?;
+        let signer = ecdsa::recover(self, hash, v, r, s)?;
         Ok(signer)
     }
 }
diff --git a/lib/motsu/README.md b/lib/motsu/README.md
@@ -13,7 +13,7 @@ Annotate tests with `#[motsu::test]` instead of `#[test]` to get access to VM
 affordances.
 
 Note that we require contracts to implement `stylus_sdk::prelude::StorageType`.
-This trait is typically implemented by default with `stylus_proc::sol_storage`
+This trait is typically implemented by default with `stylus_proc::sol_storage` 
 or `stylus_proc::storage` macros.
 
 ```rust