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

workaround for BadToken.transfer #74

Merged
merged 9 commits into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@Velenir
Copy link
Contributor

Velenir commented Jan 11, 2019

Adds safeTranfer function for tokens that implement ERC20 but with transfer/transferFrom functions not returning bool

@Velenir Velenir requested review from anxolin and josojo Jan 11, 2019

@anxolin
Copy link
Contributor

anxolin left a comment

Can you define it in the base contracts and inherit the functionality?

Velenir added some commits Jan 11, 2019

@anxolin
Copy link
Contributor

anxolin left a comment

Can we create the PR to merge to release/2.0.0

@Velenir Velenir changed the base branch from master to release/2.0 Jan 11, 2019

Velenir added some commits Jan 11, 2019

@Velenir

This comment has been minimized.

Copy link
Contributor Author

Velenir commented Jan 11, 2019

Check by reverting and reinstating changes in DutchExchange.sol and running

npx truffle test test/dutchExchange-depositWithdrawBadToken.js
@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 14, 2019

I'll merge to release/2.0, but @josojo it's good to have also your approval. This PR is to address a security concern that would get tokens stuck in case the ERC20 implementation is not correct (no return the success of the operation)

More info here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 14, 2019

Checked that we also can deploy the contracts to rinkeby with this version ;)

@anxolin anxolin merged commit 7516d68 into release/2.0 Jan 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@josojo

This comment has been minimized.

Copy link
Contributor

josojo commented Jan 15, 2019

Isn't there a solution from zepplin or someone else for the safeTransfer, which we could just important, rather than having our own codebase?
@Velenir @anxolin

@josojo

This comment has been minimized.

Copy link
Contributor

josojo commented Jan 15, 2019

Yes, the code should be safe, approved

@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 15, 2019

I agree with @josojo, in the future, at the same time we start using their abstractions for the tokens, we should use the "standard utils" they provide, like math, or safe transfers

Thanks for the review

@anxolin anxolin deleted the fix/BadToken_transfer branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment