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

Update transferFrom to modify allowance in-between hook calls. #1751

Merged
merged 2 commits into from
May 16, 2019

Conversation

nventuro
Copy link
Contributor

Fixes an issue in the current release candidate ERC777 implementation, found by @guylando in #1749.

transferFrom now updates allowances after calling the send hook, but before calling the receive hook.

I also, uh, did a small refactor and changed some variable names while figuring out the best way to introduce this. Sorry!

The code looks somewhat weird at the moment (e.g. I'd expect to find similarities in _transfer and _send which aren't there), but all of this will make more sense once we make the ERC20 extension opt-in.

We also need to add new tests cases for this, by logging extra data in our mock sender and receiver contracts, and possibly creating new behavior-like functions that check this on ERC20 calls (including transferFrom). This may be something we want to leave for when we extract built-in ERC20 support out of the base ERC777 contract, since all of the associated ERC20 testing code will need to be changed at that point.

@nventuro nventuro added bug contracts Smart contract code. labels May 14, 2019
@nventuro nventuro added this to the v2.3 milestone May 14, 2019
@nventuro nventuro requested a review from frangio May 14, 2019 21:03
@nventuro
Copy link
Contributor Author

Turns out the ERC20 behaviour didn't check for from being zero in transferFrom :/ We should be at 100% coverage again now.

@guylando
Copy link

good catch, I hope nothing else is hiding in there hehe

@guylando
Copy link

guylando commented May 14, 2019

By the way I don't see why would anyone want to create a non-ERC20 compatible ERC777 token in the near 6-12 months. The tooling for ERC777 compared to ERC20 are far behind + exchange support, etc.
I would suggest not to waste time on that until ERC777 gets more support, and by then maybe some new standard will come which will make creating non-ERC20 compatible ERC777 token irrelevant.
Even one of the ERC777 authors recommended to make it ERC20 compatible here: ethereum/EIPs#777 (comment)

More than that: creating non-ERC20 compatible token in OpenZeppelin might encourage mistakes for tokens creators creating non-ERC20 compatible tokens without being aware of the consequences.

@guylando
Copy link

isOperatorFor is used both for operatorSend and operatorBurn which seems surprising in the sense that if someone authorizes an operator to send his tokens, I am not sure he also wants to authorize the operator to burn his tokens (in most cases)

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to review this a bit more.

contracts/token/ERC777/ERC777.sol Show resolved Hide resolved
contracts/token/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/token/ERC777/ERC777.sol Outdated Show resolved Hide resolved
@nventuro
Copy link
Contributor Author

CI will fail until #1752 is merged, due to different error messages being emitted here and in ERC20.

@nventuro nventuro reopened this May 16, 2019
@nventuro
Copy link
Contributor Author

I ended up re-doing the changes from master, and restraining myself to only apply the relevant changes 😅 I'll probably open a second PR with minor renames soon.

@nventuro nventuro requested review from frangio and removed request for frangio May 16, 2019 16:09
@guylando
Copy link

@nventuro maybe check that address is not (case insensitive) 0xdcc703c0E500B653Ca82273B7BFAd8045D85a470 in addition to 0x0. That address is the hash of the empty/zero public key. See: https://www.reddit.com/r/ethereum/comments/493cys/if_your_address_is/

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR! Short and sweet.

Can you add a pending test for the ordering of allowance updates and hook invocations?

it('updates allowance before calling received hook');

contracts/token/ERC777/ERC777.sol Outdated Show resolved Hide resolved
contracts/token/ERC777/ERC777.sol Show resolved Hide resolved
contracts/token/ERC777/ERC777.sol Show resolved Hide resolved
@nventuro
Copy link
Contributor Author

@frangio it's not a matter of a single it, there are many things that need to be tested (including the sending of the correct values, which I missed in the last commit). I created #1753 for this.

@nventuro nventuro merged commit 2ccc12b into OpenZeppelin:master May 16, 2019
@nventuro nventuro deleted the erc777-state-change branch May 16, 2019 19:01
nventuro added a commit that referenced this pull request May 16, 2019
* Fix transferFrom not updating allowance before calling receiver.

* Fix from being passed as operator.

(cherry picked from commit 2ccc12b)
@nventuro
Copy link
Contributor Author

@guylando

isOperatorFor is used both for operatorSend and operatorBurn which seems surprising in the sense that if someone authorizes an operator to send his tokens, I am not sure he also wants to authorize the operator to burn his tokens (in most cases)

Anyone that can send tokens can also, e.g, send them to addresses for which no-one has the private keys, which is pretty much a burn, as far as the user is concerned. Anyway, the EIP is very clear in this aspect, so this is not for us to decide.

maybe check that address is not (case insensitive) 0xdcc703c0E500B653Ca82273B7BFAd8045D85a470 in addition to 0x0. That address is the hash of the empty/zero public key. See: https://www.reddit.com/r/ethereum/comments/493cys/if_your_address_is/

Yeah I've thought about this, but the list of addresses that may be accidentally called is sadly way too large. The zero address is special because in Solidity uninitialized addresses hold that value, so it is easier to accidentally use it.

@guylando
Copy link

@nventuro 0x0 is the default from solidity side, 0xdcc703c0E500B653Ca82273B7BFAd8045D85a470 is the default (hash of empty) from the dapp side (as described in the reddit link where a common app bug can result in this address). So seems like the two main ones, can you please provide one other example of a popular bug-wise address which is invalid?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contracts Smart contract code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants