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

Removed receiverData and `onReceive` callback from ERC721 proxy. #877

Merged
merged 5 commits into from Jul 22, 2018

Conversation

Projects
None yet
3 participants
@hysz
Copy link
Contributor

hysz commented Jul 14, 2018

Description

In the current proxy implementation, an ERC721 taker has the opportunity to execute code on invalid state.

A transfer looks like this:

TraderA holds TokenA
TraderB holds TokenB

1. TraderA creates an order to trade TokenA for TokenB.
2. TraderB fills this order.

The settlement workflow looks like this:

#1
TokenA is transferred to TraderB.
TraderB holds **both** TokenA and TokenB.
TraderB.onReceived is called.

#2
TokenB is transferred to TraderA.
TraderA holds TokenB; TraderB holds TokenA.
TraderA.onReceived is called.

This could pose a problem if, for example, TokenA and TokenB were NFT’s that when combined allow you to perform some restricted operation.

A more robust strategy could be to provide generalized callbacks that are executed after settlement, and which can be used with any proxy. Perhaps this is something we can look into for V3.

Testing instructions

All existing tests pass. A new test was created to verify that the ERC721 onReceived callback is not getting called.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Change requires a change to the documentation.
  • Added tests to cover my changes.
  • Added new entries to the relevant CHANGELOG.jsons.
  • Labeled this PR with the 'WIP' label if it is a work in progress.
  • Labeled this PR with the labels corresponding to the changed package.

@hysz hysz requested review from abandeali1 and Recmo Jul 14, 2018

@@ -83,34 +83,26 @@ contract ERC721Proxy is
// WARNING: The ABIv2 specification allows additional padding between
// the Params and Data section. This will result in a larger
// offset to assetData.

This comment has been minimized.

@hysz

hysz Jul 14, 2018

Contributor

Question: Do you think we should change the ID of this proxy?

It is currently:

bytes4 constant internal PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)"));

We could change this to:

bytes4 constant internal PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256)"));

I've left it unchanged for now so that forwarding contracts can include the additional bytes, if they'd like to implement callbacks.

This comment has been minimized.

@abandeali1

abandeali1 Jul 15, 2018

Member

If we leave the bytes param in here, we should also add it to the ERC20 PROXY_ID. This is probably the most accurate since no length checks are enforced in either proxy, and any extra data is technically part of what is being signed.

The argument for removing it is that signers/wallets are going to end up displaying a null bytes field the vast majority of the time. It's also pretty confusing considering any extra data doesn't have any functionality unless used in conjunction with an external contract. There is also nothing stopping anyone from adding extra bytes that are not ABI encoded (the decoder will expect an offset and a length).

I think it should probably be left out. Signers can still display and warn users about the extra data.

This comment has been minimized.

@hysz

hysz Jul 16, 2018

Contributor

Yeah, I suppose in both cases the extra data would just be displayed as an unstructured byte array. I'll make the update.

This comment has been minimized.

@hysz

hysz Jul 16, 2018

Contributor
  • Updated
@@ -252,7 +252,7 @@ describe('Asset Transfer Proxies', () => {
expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress);
});

it('should call onERC721Received when transferring to a smart contract without receiver data', async () => {
it('should not call onERC721Received when transferring to a smart contract', async () => {

This comment has been minimized.

@hysz

hysz Jul 14, 2018

Contributor

Left one callback sanity check in place. Removed the others.

@hysz hysz requested a review from dekz Jul 14, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 14, 2018

Coverage Status

Coverage increased (+0.02%) to 83.959% when pulling 9aa49e5 on feature/contracts/removeERC721Callback into e2fb49a on v2-prototype.

@hysz hysz force-pushed the feature/contracts/removeERC721Callback branch 4 times, most recently Jul 17, 2018

@hysz hysz force-pushed the feature/contracts/removeERC721Callback branch Jul 18, 2018

@hysz hysz force-pushed the feature/contracts/removeERC721Callback branch to 36c27bd Jul 18, 2018

hysz added some commits Jul 18, 2018

@abandeali1 abandeali1 merged commit 195d11f into v2-prototype Jul 22, 2018

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: static-tests Your tests passed on CircleCI!
Details
ci/circleci: submit-coverage Your tests passed on CircleCI!
Details
ci/circleci: test-contracts-ganache Your tests passed on CircleCI!
Details
ci/circleci: test-contracts-geth Your tests passed on CircleCI!
Details
ci/circleci: test-rest Your tests passed on CircleCI!
Details
coverage/coveralls Coverage increased (+0.02%) to 83.959%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment