Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Turn All Call::new_in into Call::new (i.e. Stop Supporting Reentrancy) #440

Merged
merged 14 commits into from
Dec 12, 2024
Merged
9 changes: 8 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- 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
- Update internal functions of `Erc721` and `Erc721Consecutive` accept reference to `Bytes`. #437

### 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

Expand Down
8 changes: 1 addition & 7 deletions contracts/src/finance/vesting_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
block,
call::{self, call, Call},
contract, evm, function_selector,
storage::TopLevelStorage,
stylus_proc::{public, sol_storage, SolidityError},
};

Expand Down Expand Up @@ -112,11 +111,6 @@
}
}

/// 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 {
Expand Down Expand Up @@ -346,7 +340,7 @@
) -> Result<U256, Self::Error>;
}

#[public]

Check warning on line 343 in contracts/src/finance/vesting_wallet.rs

View workflow job for this annotation

GitHub Actions / nightly / doc

unexpected `cfg` condition value: `export-abi`

Check warning on line 343 in contracts/src/finance/vesting_wallet.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`

Check warning on line 343 in contracts/src/finance/vesting_wallet.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`
impl IVestingWallet for VestingWallet {
type Error = Error;

Expand Down Expand Up @@ -420,7 +414,7 @@

let owner = self.ownable.owner();

call(Call::new_in(self).value(amount), owner, &[])?;
call(Call::new().value(amount), owner, &[])?;

evm::log(EtherReleased { amount });

Expand Down
2 changes: 1 addition & 1 deletion contracts/src/token/erc1155/extensions/supply.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
}
}

#[public]

Check warning on line 83 in contracts/src/token/erc1155/extensions/supply.rs

View workflow job for this annotation

GitHub Actions / nightly / doc

unexpected `cfg` condition value: `export-abi`

Check warning on line 83 in contracts/src/token/erc1155/extensions/supply.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`

Check warning on line 83 in contracts/src/token/erc1155/extensions/supply.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`
impl IErc1155 for Erc1155Supply {
type Error = erc1155::Error;

Expand Down Expand Up @@ -279,7 +279,7 @@
self._update(from, to, ids.clone(), values.clone())?;

if !to.is_zero() {
self.erc1155._check_on_erc1155_received(
Erc1155::_check_on_erc1155_received(
msg::sender(),
from,
to,
Expand Down
12 changes: 2 additions & 10 deletions contracts/src/token/erc1155/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
call::{self, Call, MethodError},
evm, function_selector, msg,
prelude::{public, sol_storage, AddressVM, SolidityError},
storage::TopLevelStorage,
};

use crate::utils::{
Expand Down Expand Up @@ -189,11 +188,6 @@
}
}

/// 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 {
Expand Down Expand Up @@ -384,7 +378,7 @@
) -> Result<(), Self::Error>;
}

#[public]

Check warning on line 381 in contracts/src/token/erc1155/mod.rs

View workflow job for this annotation

GitHub Actions / nightly / doc

unexpected `cfg` condition value: `export-abi`

Check warning on line 381 in contracts/src/token/erc1155/mod.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`

Check warning on line 381 in contracts/src/token/erc1155/mod.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`
impl IErc1155 for Erc1155 {
type Error = Error;

Expand Down Expand Up @@ -557,7 +551,7 @@
self._update(from, to, ids.clone(), values.clone())?;

if !to.is_zero() {
self._check_on_erc1155_received(
Erc1155::_check_on_erc1155_received(
msg::sender(),
from,
to,
Expand Down Expand Up @@ -767,7 +761,6 @@
///
/// # 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.
Expand All @@ -786,7 +779,6 @@
/// 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,
Expand All @@ -798,7 +790,7 @@
}

let receiver = IERC1155Receiver::new(to);
let call = Call::new_in(self);
let call = Call::new();
let result = match details.transfer {
Transfer::Single { id, value } => receiver
.on_erc_1155_received(call, operator, from, id, value, data),
Expand Down
8 changes: 1 addition & 7 deletions contracts/src/token/erc20/extensions/permit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
use stylus_sdk::{
block,
prelude::StorageType,
storage::TopLevelStorage,
stylus_proc::{public, sol_storage, SolidityError},
};

Expand Down Expand Up @@ -79,12 +78,7 @@
}
}

/// 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]

Check warning on line 81 in contracts/src/token/erc20/extensions/permit.rs

View workflow job for this annotation

GitHub Actions / nightly / doc

unexpected `cfg` condition value: `export-abi`

Check warning on line 81 in contracts/src/token/erc20/extensions/permit.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`

Check warning on line 81 in contracts/src/token/erc20/extensions/permit.rs

View workflow job for this annotation

GitHub Actions / ubuntu / beta

unexpected `cfg` condition value: `export-abi`
impl<T: IEip712 + StorageType> Erc20Permit<T> {
/// Returns the current nonce for `owner`.
///
Expand Down Expand Up @@ -174,7 +168,7 @@

let hash: B256 = self.eip712.hash_typed_data_v4(struct_hash);

let signer: Address = ecdsa::recover(self, hash, v, r, s)?;
let signer: Address = ecdsa::recover(hash, v, r, s)?;

if signer != owner {
return Err(ERC2612InvalidSigner { signer, owner }.into());
Expand Down
6 changes: 0 additions & 6 deletions contracts/src/token/erc20/utils/safe_erc20.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use stylus_sdk::{
contract::address,
evm::gas_left,
function_selector,
storage::TopLevelStorage,
stylus_proc::{public, sol_storage, SolidityError},
types::AddressVM,
};
Expand Down Expand Up @@ -80,11 +79,6 @@ sol_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.
Expand Down
3 changes: 0 additions & 3 deletions contracts/src/token/erc721/extensions/consecutive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use alloy_sol_types::sol;
use stylus_sdk::{
abi::Bytes,
evm, msg,
prelude::TopLevelStorage,
stylus_proc::{public, sol_storage, SolidityError},
};

Expand Down Expand Up @@ -134,8 +133,6 @@ pub enum Error {
ForbiddenBatchBurn(ERC721ForbiddenBatchBurn),
}

unsafe impl TopLevelStorage for Erc721Consecutive {}

// ************** ERC-721 External **************

#[public]
Expand Down
11 changes: 3 additions & 8 deletions contracts/src/token/erc721/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,6 @@ sol_storage! {
}
}

/// 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 {
Expand Down Expand Up @@ -1097,7 +1092,7 @@ impl Erc721 {
///
/// # Arguments
///
/// * `&mut self` - Write access to the contract's state.
/// * `&self` - Read 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.
Expand All @@ -1111,7 +1106,7 @@ impl Erc721 {
/// interface id or returned with error, then the error
/// [`Error::InvalidReceiver`] is returned.
pub fn _check_on_erc721_received(
&mut self,
&self,
operator: Address,
from: Address,
to: Address,
Expand All @@ -1123,7 +1118,7 @@ impl Erc721 {
}

let receiver = IERC721Receiver::new(to);
let call = Call::new_in(self);
let call = Call::new();
let result = receiver.on_erc_721_received(
call,
operator,
Expand Down
26 changes: 5 additions & 21 deletions contracts/src/utils/cryptography/ecdsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use alloy_primitives::{address, uint, Address, B256, U256};
use alloy_sol_types::{sol, SolType};
use stylus_sdk::{
call::{self, Call, MethodError},
storage::TopLevelStorage,
0xNeshi marked this conversation as resolved.
Show resolved Hide resolved
stylus_proc::SolidityError,
};

0xNeshi marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -71,7 +70,6 @@ sol! {
///
/// # Arguments
///
/// * `storage` - Write access to storage.
/// * `hash` - Hash of the message.
/// * `v` - `v` value from the signature.
/// * `r` - `r` value from the signature.
Expand All @@ -87,16 +85,10 @@ sol! {
/// # Panics
///
/// * If the `ecrecover` precompile fails to execute.
pub fn recover(
storage: &mut impl TopLevelStorage,
hash: B256,
v: u8,
r: B256,
s: B256,
) -> Result<Address, Error> {
pub fn recover(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(storage, hash, v, r, s)
_recover(hash, v, r, s)
}

/// Calls `ecrecover` EVM precompile.
Expand All @@ -106,7 +98,6 @@ pub fn recover(
///
/// # Arguments
///
/// * `storage` - Write access to storage.
/// * `hash` - Hash of the message.
/// * `v` - `v` value from the signature.
/// * `r` - `r` value from the signature.
Expand All @@ -122,13 +113,7 @@ pub fn recover(
/// # Panics
///
/// * If the `ecrecover` precompile fails to execute.
fn _recover(
storage: &mut impl TopLevelStorage,
hash: B256,
v: u8,
r: B256,
s: B256,
) -> Result<Address, Error> {
fn _recover(hash: B256, v: u8, r: B256, s: B256) -> Result<Address, Error> {
let calldata = encode_calldata(hash, v, r, s);

if v == 0 || v == 1 {
Expand All @@ -139,9 +124,8 @@ fn _recover(
return Err(ECDSAInvalidSignature {}.into());
}

let recovered =
call::static_call(Call::new_in(storage), ECRECOVER_ADDR, &calldata)
.expect("should call `ecrecover` precompile");
let recovered = call::static_call(Call::new(), ECRECOVER_ADDR, &calldata)
.expect("should call `ecrecover` precompile");

let recovered = Address::from_slice(&recovered[12..]);

Expand Down
2 changes: 1 addition & 1 deletion examples/ecdsa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ impl ECDSAExample {
r: B256,
s: B256,
) -> Result<Address, Vec<u8>> {
let signer = ecdsa::recover(self, hash, v, r, s)?;
let signer = ecdsa::recover(hash, v, r, s)?;
Ok(signer)
}
}
Loading