Issue with Test test_transfer_from_from_zero_address Using OpenZeppelin's Tests as Reference

Support Contracts
,
1

Hello,

I've been using OpenZeppelin's tests as a guide for my own, and I've encountered an issue with the test_transfer_from_from_zero_address test. Previously, this test was passing successfully, but now it's failing with a panic:

reflect_cairo::tests::reflect::test_reflect::test_transfer_from_from_zero_address - panicked with [39879774624085075084607933104993585622903 ('u256_sub Overflow'), ].

Here's the test code:

#[test]
#[available_gas(2000000)]
#[should_panic(expected: ('Transfer from the zero address',))]
fn test_transfer_from_from_zero_address() {
    let mut state = setup();
    ERC20Impl::transfer_from(ref state, Zeroable::zero(), RECIPIENT(), VALUE);
}

I'm curious if using Zeroable::zero() as the 'from' address might be causing a lower-level panic than expected. Any insights into why this test might now be failing would be greatly appreciated.

Thank you in advance for your help.

1 Like
2

Sorry I don't quite understand, whats the context of this question? That's a test from the OpenZeppelin Contracts for Cairo suite, are you modifying the repo? How are you modifying it? What's the difference between before and after?

1 Like
3

Thank you for your patience. I want to clarify my concern regarding the OpenZeppelin Contracts for Cairo, specifically the ERC20 v0.7.0 _transfer function. In the source code, there is an assertion assert(!sender.is_zero(), Errors::TRANSFER_FROM_ZERO);, suggesting that if the sender is the zero address, the function should panic with the message ERC20: transfer from 0.

However, in the OpenZeppelin test suite, the fn test_transfer_from_from_zero_address() test, instead of panicking with the expected ERC20: transfer from 0 message, is actually panicking with a u256_sub Overflow message. This seems to indicate that a lower-level operation is failing before the check for

the zero address.

My question is, why is the test panicking with u256_sub Overflow instead of the expected ERC20: transfer from 0? Is there a lower-level operation happening before the evaluation of the zero address as the 'from' address in the transfer, causing this discrepancy?

1 Like
4

You haven't provided the code of that function, but at least by its name - most likely no.

Same goes for function RECIPIENT().

If anything could cause the error in context, my guess is that it would be function setup().

Can you please provide the code of that function?

1 Like
5

Yes, sure. I will provide how you have it in your code for erc20 v0.7.0 and its tests.

OZ _transfer function:

        fn _transfer(
            ref self: ContractState,
            sender: ContractAddress,
            recipient: ContractAddress,
            amount: u256
        ) {
            assert(!sender.is_zero(), Errors::TRANSFER_FROM_ZERO);
            assert(!recipient.is_zero(), Errors::TRANSFER_TO_ZERO);
            self.ERC20_balances.write(sender, self.ERC20_balances.read(sender) - amount);
            self.ERC20_balances.write(recipient, self.ERC20_balances.read(recipient) + amount);
            self.emit(Transfer { from: sender, to: recipient, value: amount });
        }

We see we have an assert with the text Errors::TRANSFER_FROM_ZERO if the assert's condition is not satisfied.

assert(!sender.is_zero(), Errors::TRANSFER_FROM_ZERO);

Now let's go to the tests. I found the function fn test_transfer_from_from_zero_address() :

#[test]
#[available_gas(2000000)]
#[should_panic(expected: ('u256_sub Overflow',))]
fn test_transferFrom_from_zero_address() {
    let mut state = setup();
    ERC20CamelOnlyImpl::transferFrom(ref state, Zeroable::zero(), RECIPIENT(), VALUE);
}

Here is where I get confused. I thought we were expecting something related to Errors::TRANSFER_FROM_ZERO or ERC20: transfer from 0 to panic the test, but instead we are expecting u256_sub Overflow to panic the test.

Sorry for all the confusions.

1 Like
6

Did you actually read my response?

Here it is again, please try to follow:

1 Like
7

Here is the setup() function as implemented in my code:

fn setup() -> ERC20::ContractState {
    let mut state = STATE();
    ERC20::constructor(ref state, NAME, SYMBOL, SUPPLY, OWNER());
    utils::drop_event(ZERO());
    state
}

It is exactly as you have in your tests, same constants values too.

1 Like
8

Yes, I have modified OZ tests, the test of erc20 v0.7.0.

The test function is test_transfer_from_from_zero_address(). It is almost like yours but mine expects a panic message "Transfer from the zero address". This panic message comes from the _transfer function in my code.

My _transfer function is like OZ, but with different panic messages:

mine:

assert(!sender.is_zero(), 'Transfer from the zero address');

OZ:

assert(!sender.is_zero(), Errors::TRANSFER_FROM_ZERO);

Then, when I go to OZ tests to check for this condition:

#[should_panic(expected: ('u256_sub Overflow',))]
fn test_transfer_from_from_zero_address() {
    let mut state = setup();
    ERC20Impl::transfer_from(ref state, Zeroable::zero(), RECIPIENT(), VALUE);
}

The test expects a u256_sub Overflow panic message instead of Transfer from the zero address or ERC20: transfer from 0. Why do we expect this message and not the one me and you have defined at _transfer function.

Sorry for any confusion.

1 Like
9

Because function transfer_from calls _spend_allowance before it calls _transfer:

self._spend_allowance(sender, caller, amount);
self._transfer(sender, recipient, amount);

And function _spend_allowance performs an underflowing subtraction:

let current_allowance = self.ERC20_allowances.read((owner, spender));
self._approve(owner, spender, current_allowance - amount);

Because the value of current_allowance is obviously 0, and the value of amount is larger than 0.

1 Like
10

Thank you! understood the issue in the _spend_allowance function :+1:

I think the test test_transferFrom_from_zero_address is evaluating a condition different from what its name suggests. The name implies testing the scenario of transferring from a zero address, but the test is evaluating an underflow in _spend_allowance before that.

Thanks for the patience.

3 Likes