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

Signature Malleability: #1622

Merged
merged 11 commits into from Mar 7, 2019

Conversation

@tbocek
Copy link
Contributor

commented Jan 23, 2019

If you allow both values 0/1 and 27/28, you allow two different signatures both resulting in a same valid recovery. (r,s,0/1) and (r,s,27/28) would both be valid, recover the same public key and sign the same data. Furthermore, given (r,s,0/1), (r,s,27/28) can be constructed by anyone. In my contracts, I removed line 37 to line 39.

I further checked with geth, and in the transaction and ethapi, only 27/28 is considered. Thus, I added a function fixSignature() to convert 0/1 to 27/28 resulting from web3.eth.sign().

The current implementation opens attacks on contracts that rely where a signature is unique. This is for example ERC 865 as implemented in this pull request.

Transaction Malleability:
If you allow for both values 0/1 and 27/28, you allow two different
signatures both resulting in a same valid recovery. (r,s,0/1) and
(r,s,27/28) would both be valid, recover the same public key and sign
the same data. Furthermore, given (r,s,0/1), (r,s,27/28) can be
constructed by anyone.

@nventuro nventuro added the bug label Jan 23, 2019

@nventuro nventuro requested review from shrugs and frangio Jan 23, 2019

@tbocek tbocek changed the title Transaction Malleability: Signature Malleability: Jan 23, 2019

@frangio

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Hi @tbocek. Thanks for the report. I can see how this is a problem.

If I recall correctly accepting both 0/1 and 27/28 is very common, though. Have you reported this to web3 or anywhere else?

@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

@frangio I have not reported it elsewhere. I looked around and it seems to be a known issue in web3. I think the proper way could be to use web3.eth.accounts.sign (https://web3js.readthedocs.io/en/1.0/web3-eth-accounts.html#sign), but I have not tested it.

As far as I can tell, geth only supports 27/28.

@tinchoabbate

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Further supporting @tbocek , in the Ethereum's yellow paper, see Appendix F "Signing transactions", it is stated that among other conditions for an ECDSA signature to be valid, v must be 27 or 28.

@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

This bugs needs further fixing. After discussion in ERC865, there also needs to be a check that s needs to be lower than the half order as it is still vulnerable to signature malleability. In EIP-2 they state:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures.

That means ecrecover sees both signatures as valid, with lower s and upper s. So, my idea is the check for that as well and only allow lower s.

Alternatively, an other fix could be in the function recover() to not check and make sure that the signature is unique, but to return with the address in addition a unique signature (if s upper -> s=N-s, v+27 if v<27), that the caller of recover() can use, and knows its unique. However, I leaning towards checking only as this is seems less error prone as you don't have two kind of signatures in the application code.

@cwhinfrey

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@tbocek I was just going to mention that, as you said, the signatures are still malleable because both lower and upper s values are accepted. I have no strong opinions regarding accepting versions 0/1 but am in favor of allowing malleable signatures but possibly adding a warning for developers to never use them as unique identifiers as @yondonfu states in this post. https://yondon.blog/2019/01/01/how-not-to-use-ecdsa/

Transaction Malleability:
EIP-2 still allows signature malleabality for ecrecover(), remove this
possibility and force the signature to be unique.
@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@cwhinfrey I'm for either all or nothing. Fixing only 0/1 does not solve the problem of signature malleability. I now added a check to allow only s-values for lower than the half order in this pull request. This way a signature needs to be unique. That also means if you are using an outside signing library that creates non-unique signatures, the signatures need to be converted first.

I would rather add a warning, that recover() only accepts unique signatures, and if you have a library that creates values with 0/1 and s-values higher that the half order, either replace the order check with:

// Version of signature should be 27 or 28, but 0 and 1 are also possible versions
if (v < 27) {
    v += 27;
}

or convert the signatures from that library to a unique signature.

@nventuro nventuro self-assigned this Feb 27, 2019

@nventuro nventuro removed the request for review from shrugs Feb 27, 2019

@nventuro nventuro added this to the v2.2 milestone Feb 27, 2019

@nventuro nventuro added the contracts label Feb 27, 2019

@nventuro

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Further supporting @tbocek , in the Ethereum's yellow paper, see Appendix F "Signing transactions", it is stated that among other conditions for an ECDSA signature to be valid, v must be 27 or 28.

Appendix F also mentions that the valid range for s is 0 < s < secp256k1n ÷ 2 + 1. This is IMO more than enough to restrict both the s and v values as done by @tbocek here.

@nventuro
Copy link
Member

left a comment

Thanks a lot @tbocek! Sorry for taking so long to review this, you caught me in the middle of my vacations.

Left a couple comments regarding style, etc., but overall the changes are quite simple - what's important is the research you carried out :) I can take care of getting this up to speed so you don't have to waste time reformatting, etc. - just let me know!

contracts/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
contracts/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
test/helpers/sign.js Outdated Show resolved Hide resolved
test/cryptography/ECDSA.test.js Outdated Show resolved Hide resolved
test/helpers/sign.js Outdated Show resolved Hide resolved
@cwhinfrey

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Appendix F also mentions that the valid range for s is 0 < s < secp256k1n ÷ 2 + 1. This is IMO more than enough to restrict both the s and v values as done by @tbocek here.

Happy with this going either way but I wanted to point out that I believe Appendix F is specifically talking about singing transactions. EIP-2 which introduced that restriction also says this:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered 
invalid. The ECDSA recover precompiled contract remains unchanged and will keep 
accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin
signatures.
@nventuro

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I'd move for making all signing schemes as equal as possible though: it'd be very annoying if we accepted data that was signed in a way that wouldn't work for a regular transaction. Additionally, the proposed use case (recovering old BTC signatures) sounds so obscure that I'd expect whoever is working on that to implement it themselves, and wouldn't compromise on this point here to support it.

That said, I'm definitely not an expert in this matter and it is entirely possible that I'm misunderstanding the situation: if that's the case, please let me know.

@cwhinfrey

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Additionally, the proposed use case (recovering old BTC signatures) sounds so obscure

I was thinking the same thing. 😆 I think that all makes a lot of sense.

@chriseth chriseth referenced this pull request Mar 4, 2019
@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I'm guessing it made sense to leave ecrecover() unchanged in EIP-2 (allowing both s) in Ethereum as changing it could potentially break smart contracts. There may be some obscure contracts relying on this feature. Since a unique signature can be enforced by smart contracts, I'm speculating that this was the reason to leave ecrecover() unchanged and rely on developers solving that in smart contracts.

Thus, I would like to see such an unique signature enforcement by a trusted library.

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Another reason was to be able to verify bitcoin signatures.

nventuro added some commits Mar 6, 2019

@nventuro
Copy link
Member

left a comment

Thanks a lot @tbocek! In the interest of putting this into the 2.2 release, I took the liberty of pushing some commits directly to your fork, with minor formatting changes, changelog entry, etc.

Also thanks to @tinchoabbate, @cwhinfrey and everyone who participated in this discussion!

@nventuro nventuro referenced this pull request Mar 7, 2019
@nventuro
Copy link
Member

left a comment

I added a hardcoded test for a high-s value signature, but we should eventually work on those. Created #1667.

@nventuro nventuro merged commit 79dd498 into OpenZeppelin:master Mar 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@stevenlcf stevenlcf referenced this pull request Apr 30, 2019

SantiRSK added a commit to rsksmart/openzeppelin-solidity that referenced this pull request May 14, 2019

Add rsk (#2)
* Release candidate v2.1.0-rc.1

* Release v2.1.0 solc 0.5.x (OpenZeppelin#1568)

* Now compiling in a separate directory using truffle 5.

* Ported to 0.5.1, now compiling using 0.5.1.

* test now also compiles using the truffle 5 hack.

* Downgraded to 0.5.0.

* Sorted scripts.

* Cleaned up the compile script a bit.

* remove linux-specific mktemp usage (OpenZeppelin#1571)


(cherry picked from commit 7361ffd)

* remove note about 2.0 being experimental (OpenZeppelin#1565)

(cherry picked from commit 5036741)

* Updated PausableCrowdsale to require solc ^0.4.24 (OpenZeppelin#1567)

(cherry picked from commit 73cbad2)

* Updated changelog to reflect 0.5 support.

* Release candidate v2.1.0-rc.2

* Separate unsigned and signed safemath libraries (OpenZeppelin#1588)

* separate unsigned and signed safemath libraries

* update changelog entry for SignedSafeMath

* Rename WhitelisterRole to WhitelistAdminRole. (OpenZeppelin#1589)

* Rename WhitelisterRole to WhitelistAdminRole.

* Update WhitelistAdmin changelog entry.

* Replaced Solium in favor of Solhint (OpenZeppelin#1575)

* Adding solhint, working on style fixes.

* Upgraded to solhint 1.5.0.

* Removed all references to Solium

* Updated mocks to make the pass the new linter rules.

* Reformatted the .solhint.json file a bit.

* Removed Solium configuration files.

* Remove Solium dependency.

* Add comment explaing disabled time rule in TokenVesting.

* Revert to the old (ugly?) style.

* Revert SignatureBouncerMock style.

* Fix ERC165InterfacesSupported interface.

* silence npm output (OpenZeppelin#1590)

* Updated 2.1 release date.

* Release v2.1.0

* Added 2.1.1 changelog entry.

* Release v2.1.1

* Add 2.2.0 changelog entry.

* Migration to truffle 5 (and web3 1.0 (and BN)) (OpenZeppelin#1601)

* Now compiling using truffle 5.

* Migrated some test files, missing BN scientific notation usage.

* Now using BN time values.

* Migrate ERC20 tests.

* Migrate all ERC20 tests.

* Migrate utils, payment and ownership tests.

* All tests save ERC721 migrated.

* Migrated ERC721 tests.

* Fix lint errors.

* Delete old test helpers.

* Fix remaining crowdsale tests.

* Fix signature bouncer tests.

* Update how constants is used.

* Compile script pre-removes the build dir.

* Fix SafeMath tests.

* Revert "Compile script pre-removes the build dir."

This reverts commit 247e745.

* Fix linter errors.

* Upgrade openzeppelin-test-helpers dependency.

* Update openzeppelin-test-helpers dependency.

* Define math constants globally.

* Remove unnecessary ether unit.

* Roll back reduced ether amounts in tests.

* Remove unnecessary toNumber conversions.

* Delete compile script.

* Fixed failing test.

* Temporarily disable solidity-coverage Travis job.

* Update readme example to use 0.5.0.

* Fix warnings (OpenZeppelin#1606)

* Bump required compiler version to 0.5.2.

* Fix shadowed variable warning in ERC20Migrator.

* Rename Counter to Counters.

* Add dummy state variable to SafeERC20Helper.

* Update changelog entry.

* Fix CountersImpl name.

* Improve changelog entry.

* Only publish the test suite behavior subdirectory

* Move PublicRole.behavior to behavior directory.

* Add some barebones PublicRole.behavior documentation.

* Add changelog entry for PublicRole behavior.

* Renamed test/behavior to test/behaviors.

* Release v2.1.2

* ERC20._approve (OpenZeppelin#1609)

* Add ERC20._approve.

* Add ERC20._approve tests.

* Fix linter error.

* Require owner in _approve to be non-zero.

* Remove unnecessary SafeMath call (OpenZeppelin#1610)

* Refactor Counter to support increment and decrement.

* Move Counter out of drafts.

* Refactor ERC721 to use Counter.

* Rollback Counter returning the current value in increment and decrement.

* Update test/drafts/Counter.test.js

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Improve Counter documentation.

* Move Counter.test to utils.

* Move back Counter to drafts.

* Improve SafeMath test coverage. (OpenZeppelin#1611)

* Improve SafeMath test coverage.

* Fix linter error.

* Split testCommutative into something more sane.

* Bring back coverage report. (OpenZeppelin#1620)

* Add back solidity-coverage (using fork).

* Pin fork version.

* Unify code comments style. (OpenZeppelin#1603)

* Updated code style to no more than120 characters per line.

* Unify code comments style with Doxygen-style tags.

* Add ERC20 _setTokenURI (OpenZeppelin#1618)

* Add _setTokenURI internal.

* Rename TokenMetadata to ERC20Metadata.

* Add changelog entry for ERC20Metadata.

* Fix linter error.

* Add breaking change changelog notice.

* Fix typo in README (OpenZeppelin#1624)

* ERC20 Snapshot Impl #2 (OpenZeppelin#1617)

* ✏️ Refactor code & Refork OZ Repo

* Refactor ERC20Snapshot to use on-demand snapshots.

* Add ERC20Snapshot changelog entry.

* Move ERC20Snapshot to drafts.

* Improve changelog entry.

* Make snapshot tests clearer.

* Refactor ERC20Snapshots to use Counters.

* Refactor snapshot arrays into a struct.

* Remove .DS_Store files.

* Delete yarn.lock

* Fix linter error.

* simplify gitignore entry

* Add a link to the minime token

* Add the @dev tag

* Fix typo: snapshoted

* Fix typo: grater

* Fix typo: to be find (OpenZeppelin#1642)

* Update to preferred citation formation for ERC-721

* Use canonical EIP reference format

* Clarify the ERC20Snapshot contract comment (OpenZeppelin#1638)

* Add usage docs to ERC20 Snapshot (OpenZeppelin#1639)

* Fix SafeERC20.safeApprove bug, improve test coverage.

* Added PR links for 2.2.0 changelog entries.

* Add SafeERC20 bugfix changelog entry.

* Merge pull request OpenZeppelin#1647 from nventuro/safeerc20-bugfix

Fix SafeERC20.safeApprove bug

(cherry picked from commit 3111291)

* Add bugfix backport changelog entry.

* Release v2.1.3

* Merge pull request OpenZeppelin#1647 from nventuro/safeerc20-bugfix

Fix SafeERC20.safeApprove bug

(cherry picked from commit 3111291)

* Release v2.0.1

* Add the solidity linter command to the PR template (OpenZeppelin#1653)

* Add the solidity linter command to the PR template

The PR template states that a contributor should run the
Solidity/JS linters before submission. However, it only states the
command for the JS linter.

This commit adds the Solidity linter command explicitly.

* Use past tense in the list of prerequisites

* Nonfunctional typos OpenZeppelin#1643 (OpenZeppelin#1652)

* Add IntelliJ IDE config to .gitignore

* Fix variable name in ERC20 function comments

* Fix typos in Arrays function comment

* Fix typos in ownership test names

* Fix typo in Pausable test name

* Fix grammar in Ownable function comment

* Fix grammar in Crowdsale contract comment

* Fix typo in Counters contract comment

* Fix typo in ERC721Enumerable comment

* Fix typo in ERC721PausedToken test name

* Fix typo in Crowdsale function comment

* Fix typo in IncreasingPriceCrowdsale function comment

* Fix grammar in IncreasingPriceCrowdsale test name

* Fix typo in AllowanceCrowdsale test name

* Fix typo in RefundEscrow function comment

* Fix typo in ERC20Migrator contract comment

* Fix typos in SignatureBouncer comments

* Fix typo in SignedSafeMath test name

* Fix typo in TokenVesting contract comment

* Move Ownable comment from @notice section to @dev

The Ownable contract has a comment explaining that renouncing
ownership will prevent execution of functions with the onlyOwner
modifier.

This commit moves that comment to the @dev section and replaces it
with a description suitable for a generic user.

* Clarify purpose of ERC20 transfer function

* Clarify registration of ERC721Enumerable interface

* Clarify purpose of AllowanceCrowdsale test

* Increase specificity of inheritance comments

FinalizableCrowdsale and RefundableCrowsale both have comments
indicating that they are extensions of the Crowdsale contract.

This commit refines those comments to the most immediate ancestor
( TimedCrowdsale and RefundableCrowdsale respectively )

* Remove unused parameter in PaymentSplitter test

* Rename parameter in SignatureBouncer functions

The SignatureBouncer contract has modifiers to validate the
message sender is authorised to perform an action. They pass
msg.sender to internal functions as the variable `account`, but
the function comments refer to the variable as `sender`

This commit changes the variable name to `sender`

* Clarify comments in SignatureBouncer functions

The SignatureBouncer has comments that use the description
`sender` to refer to the variable `account`.

This commit updates the comments for consistency.

Maintainer Note: this reverts changes in the previous commit,
which renamed the variable `account` instead.

* Add no-return-data ERC20 support to SafeERC20. (OpenZeppelin#1655)

* Add no-return-data ERC20 support to SafeERC20.

* Add changelog entry.

* Replace abi.encodeWithSignature for encodeWithSelector.

* Remove SafeERC20 test code duplication.

* Replace assembly for abi.decode.

* Fix linter errors.

* Fix typo: "an uint256" -> "a uint256" (OpenZeppelin#1658)

Using "a" instead of "an" makes this consistent with the comment on `allowance`.

* fix weird date format (OpenZeppelin#1663)

* remove .node-version file (OpenZeppelin#1665)

* Add latest audit to repository (OpenZeppelin#1664)

* rename previous audit to date it was performed

* add latest audit

* add note about latest audit in README

* Add guard to ERC20Migrator migrate function (OpenZeppelin#1659)

* Add guard to ERC20Migrator migrate function

* Add tests for premature migration in ERC20Migrator

These tests apply to the new guard condition, but they don't
fail without it, since the functions revert anyway.
They are added for completeness and to ensure full code coverage.

* Use context block around premature migration tests

We should use context blocks for situational details
and describe for features or functions.

* Add TimedCrowdsale::_extendTime (OpenZeppelin#1636)

* Add TimedCrowdsale::_extendTime

* Add tests for TimedCrowdsale extending method

* Reverse event arguments order

* Rename method argument

* Refactor TimedCrowdsale test

* Simplify TimedCrowdsaleImpl

* Fix extendTime method behaviour to deny TimedCrowdsale re-opening after it was ended

* Append chengelog

* Update CHANGELOG.md

Co-Authored-By: k06a <k06aaa@gmail.com>

* Update contracts/crowdsale/validation/TimedCrowdsale.sol

Co-Authored-By: k06a <k06aaa@gmail.com>

* Improve tests

* Add extcodesize check to SafeERC20. (OpenZeppelin#1662)

* Add extcodesize check to SafeERC20.

* Clarify some comments.

* Replace inline assembly with Address.sol.

* Signature Malleability: (OpenZeppelin#1622)

* Transaction Malleability:
If you allow for both values 0/1 and 27/28, you allow two different
signatures both resulting in a same valid recovery. (r,s,0/1) and
(r,s,27/28) would both be valid, recover the same public key and sign
the same data. Furthermore, given (r,s,0/1), (r,s,27/28) can be
constructed by anyone.

* Transaction Malleability:
EIP-2 still allows signature malleabality for ecrecover(), remove this
possibility and force the signature to be unique.

* Added a reference to appendix F to the yellow paper and improved
comment.

* better test description for testing the version 0, which returns
a zero address

* Check that the conversion from 0/1 to 27/28 only happens if its 0/1

* improved formatting

* Refactor ECDSA code a bit.

* Refactor ECDSA tests a bit.

* Add changelog entry.

* Add high-s check test.

* Reorder 2.2.0 changelog entries.

* Release candidate v2.2.0-rc.1

* Fix changelog entry.

* Improve erc165 testing OpenZeppelin#1203 (OpenZeppelin#1666)

* Rename variable from thing to contractUnderTest

* Compute function signatures in ERC165 interfaces

The ERC165 tests currently precompute some known interface ids.
This commit extracts the interfaces into a separate object and
precomputes the individual function signatures.

This will be useful to identify contracts that support an interface
but do not implement all of the corresponding functions.

* Add tests for ERC165 interface implementations

The ERC165 tests confirm that contracts claim to support
particular interfaces ( using the supportsInterface method )

This commit extends those tests to confirm that the corresponding
functions are included in the contract ABI.

It also rewords the existing test names in order to group the
implementation tests with the corresponding interface tests.

* Remove obsolete ERC721Exists interface constant

* Fix typo in ERC20Snapshot. (OpenZeppelin#1670)

* Replace mentions of Slack for forum links. (OpenZeppelin#1671)

* Replace mentions of Slack for forum links.

* make forum lowercase

* Add API stability doc link. (OpenZeppelin#1672)

* Add API stability doc link.

* Update README.md

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Improve test script. (OpenZeppelin#1675)

* Add v2.2.0 release date.

* Release v2.2.0

* Make waiting for ganache to launch more robust. (OpenZeppelin#1683)

* Add probot/stale to the repo. (OpenZeppelin#1681)

* Remove unused return variables. (OpenZeppelin#1686)

* Draft EIP 1820 (OpenZeppelin#1677)

* Add barebones EIP1820 support.

* Update openzeppelin-test-helpers dependency to have ERC1820 support.

* Add tests for ERC1820.

* Improve inline documentation.

* Add changelog entry.

* Update test-helpers, refactor tests to use new helpers.

* Rename ERC1820 to ERC1820Implementer.

* Improve implementer docstring.

* Remove _implementsInterfaceForAddress.

* update openzeppelin-test-helpers to 0.2.0

* Update contracts/drafts/ERC1820Implementer.sol

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Fix how solidity coverage is run to allow for free events.

* Fix coverage testing script.

* Update openzeppelin-test-helpers dependency.

* Update SafeERC20.sol (OpenZeppelin#1693)

* Edit Ethereum dev framework links in README. (OpenZeppelin#1695)

Added Buidler.

* Exclude on-hold PRs and issues on stalebot. (OpenZeppelin#1696)

* Added basic punctuation to @dev docs (OpenZeppelin#1697) (OpenZeppelin#1700)

* Added basic punctuation to @dev docs (OpenZeppelin#1697)

* add missing uppercase

* add note about Counters rename in changelog (OpenZeppelin#1703)

* Remove unused files (OpenZeppelin#1698)

* Remove unused dependencies.

* Remove unused mock contracts.

* Fix from account in remove public role behaviors (OpenZeppelin#1710)

* Add WIP bot.

* Revert "Add WIP bot."

This reverts commit 07fc8c7.

* Add more extensive documentation to PaymentSplitter (OpenZeppelin#1713)

* Update PaymentSplitter.sol

* add back private function docs

* add non-zero address requirement

* add comprehensive contract-level docs

* use capital E for Ether

* Remove unnecessary SLOAD. (OpenZeppelin#1715)

* Update copyright notice

* Fix/rename anyone account OpenZeppelin#1357 (OpenZeppelin#1718)

* replacing all instances of from: anyone with from: other

* replacing all instances of from: anyone with from: other

* replacing all instances of from: anyone with from: other

* changing anyone to other

* changing anyone to other

* removing unused variables (OpenZeppelin#1719)

* removing unused variables

* undeleting the _

* removed unnecessary require and renaming of null to zero (OpenZeppelin#1717)

* removed unnecessary require

* build pipeline fix

* kept as it is

* Added require

* Feature/erc777 OpenZeppelin#1159 (OpenZeppelin#1684)

* IERC777 from specs, constants returned, up to defaultOperators. (OpenZeppelin#1159)

* IERC777 oprarator approvals (OpenZeppelin#1159)

* ERC777 oprarator approvals fixes and tests

* IERC777 send and receive with ERC820 (OpenZeppelin#1159)

* ERC777 Add burn functions and fix send functions (OpenZeppelin#1159)

* ERC777 Make expectEvent compatible with web3.js 1.0 (OpenZeppelin#1159)

* ERC777 Add ERC820 deploy script (OpenZeppelin#1159)

* ERC777 Complete implementation of ERC777 (OpenZeppelin#1159)

This implementation conforms to the current EIP

* ERC777 Update ERC820 Registry contract to final version (OpenZeppelin#1159)

* ERC777 Move contracts to 'drafts' folder (OpenZeppelin#1159)

* ERC777: Update to ERC1820 registry and linter error fix (OpenZeppelin#1159)

* ERC777: implement recent changes of EIP777 (OpenZeppelin#1159)

* ERC777 Fix formatting (OpenZeppelin#1159)

* ERC777 Update to solc 0.5.2 (OpenZeppelin#1159)

* ERC777 Fix travis CI errors (OpenZeppelin#1159)

* ERC777 Fix linter errors again... (OpenZeppelin#1159)

* ERC777 Fix unit test (OpenZeppelin#1159)

* ERC777 Fix unit test again (OpenZeppelin#1159)

* Remove extra newlines.

* Rename ERC777Base to ERC777.

* Remove 'Token' from contract names.

* Replace ops for operators.

* Move operator check out of _send.

* Remove ERC777Burnable.

* Remove ERC1820Client, now using the interface directly.

* Minor internal refactors in contracts.

* Delete extra test helpers.

* Simplified tests.

* Add basic 777 tests.

* Add granularity send test.

* Add first operator send tests.

* Add burn tests.

* Refactor send and burn tests.

* Improve send burn refactor.

* Greatly improve test module.

* Burn instead of send removed tokens.

* Add operator tests.

* Improve send tests under changing operators.

* Refactor and merge send and burn tests.

* Add missing and not-implemented tests.

* Make _burn private.

* Fix typo.

* Greatly improve tokensToSend tests.

* Refactor hook tests.

* Fix hook tests.

* Update openzeppelin-test-helpers and ERC1820 address.

* Fix natspec indentation.

* Make interface functions external.

* Remove redundant private revoke and authorize functions.

* Improved readability of if statement.

* Remove unnecessary asserts.

* Add non-one granularity test.

* Fix hook call order in _mint.

* Fix _mint not reverting on failure to implement tokensReceived.

* Remove special case in operatorFn when from is 0.

* Refactor ERC777SenderMock.

* Add tokensReceived tests.

* switch to updated ganache-cli-coverage fork

* Fix linter errors.

* Add mint tests.

* Fix linter errors.

* Fix tests.

* Update test/drafts/ERC777/ERC777.test.js

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Add changelog entry.

* Fixes/Improves constants inline documentation. (OpenZeppelin#1707)

* Fixes/Improves constants inline documentation.

* Fixed solhint error.

* Moved the comment before the variable

* Update stalebot wording and timing. (OpenZeppelin#1722)

* Fix stalebot exempt labels

* Release automation (OpenZeppelin#1720)

* Create autoamtic release script.

* Add changelog update date script.

* Improve release scripts.

* Apply suggestions from code review

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Apply suggestions from code review

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Remove moment dependency.

* New documentation setup (OpenZeppelin#1708)

* initial docsite setup

* switch from pushd to cd

* install and set up solidity-docgen

* use the docsite branch next for now

* make it clear that env var is a repository

* add a clarifying comment about a relative path

* change relative to absolute path in docsite script

* add docgen script

* add first few READMEs for contract documentation

* update solidity-docgen

* add docsite as dependency and adjust script

* update openzeppelin-docsite

* update solidity-docgen

* remove dummy text

* update docgen and docsite

* update openzeppelin-docsite

* add netlify.toml

* udpate tokens guide for 2.2

* add DOCUMENTATION.md

* Update docs/learn-about-utilities.md

Co-Authored-By: frangio <frangio.1@gmail.com>

* fix PaymentSplitter docs wording

* update solidity-docgen

* add missing ERC20 contracts

* update solidity-docgen

* trigger deploy with cleared cache

* update solidity-docgen

* update openzeppelin-docsite

* remove travis docs setup

* update openzeppelin-docsite

* switch to published solidity-docgen

* Fix linter error.

* Bump minimum Solidity version to 0.5.7 (OpenZeppelin#1724)

* Bump Solidity version to 0.5.7

* Add changelog entry.

* Add a vault to PostDeliveryCrowdsale. (OpenZeppelin#1721)

* Add a vault to PostDeliveryCrowdsale.

* Add changelog entry.

* Apply suggestions from code review

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Rename TokenVault.

* add solhint ignore directive

* make some updates on the get started guide (OpenZeppelin#1725)

* Added message string for require() (OpenZeppelin#1704)

* Error handling in ERC20 and ERC721

* Added message string for require.

* Fixed solhint errors.

* Updated PR as per issue OpenZeppelin#1709

* changes as per OpenZeppelin#1709 and openzeppelin forum.

* Changes in require statement

* Changes in require statement

* build pipeline fix

* Changes as per @nventuro's comment.

* Update revert reason strings.

* Fianal update of revert reason strings.

* WIP: Updating reason strings in test cases

* WIP: Added changes to ERC20 and ERC721

* Fixes linting errors in *.tes.js files

* Achieved 100% code coverage

* Updated the test cases with shouldFail.reverting.withMessage()

* Fix package-lock.

* address review comments

* fix linter issues

* fix remaining revert reasons

* Fix remaining revert reasons.

* Add revert reasons changelog entry.

* update links in documentation setup description

* Revert Solidity version bump. (OpenZeppelin#1729)

* fix pr number in changelog

* Fix solc-nightly job (OpenZeppelin#1732)

* update truffle to 5.0.14

* fix setup to test with solc-nightly

* switch to npx in script/test.sh

* please the linter

* rename build to prepack

* move download of nightly build to a compile script

* make compile script executable

* Update vulnerable dependencies.

* make nightly job conditional (OpenZeppelin#1737)

* update openzeppelin-docsite to fix windows issues

* Hardcode ERC777 granularity to 1, remove tests. (OpenZeppelin#1739)

* Hardcode ERC777 granularity to 1, remove tests.

* Add clarifying title comment.

* update openzeppelin-docsite to fix windows issues (part 2)

* Add ERC20 compatibility to ERC777. (OpenZeppelin#1735)

* Add ERC20 compatibility.

* Reusing ERC20 tests for ERC777.

* Improve documentation.

* Add changelog entry.

* Improved ERC20 behavior tests.

* Add revert reasons to ERC777.

* ERC20 methods allow sending tokens to contracts with no interface.

* Register ERC20 interface.

* Add comment about avoidLockingTokens.

* Improve revert reason string.

* Make ERC777 implement IERC20.

* Fix test revert string.

* Remove unnecesary require.

* Add private _transfer.

* Update contracts/drafts/ERC777/ERC777.sol

Co-Authored-By: nventuro <nicolas.venturo@gmail.com>

* Update private helper names.

* Inline keccak256 result (OpenZeppelin#1741)

* inline keccak256 result

* Update ERC777.sol

* switch hex constant style

* Update ERC777.sol

* Fix linter.

* Move ERC1820 and ERC777 out of drafts (OpenZeppelin#1742)

* Moved ERC1820 related contracts out of drafts and into introspection.

* Moved ERC777 related contracts out of drafts and into token.

* Remove broken linter rule.

* Move ERC1820 and ERC777 tests out of drafts.

* Add RSK support to latests OZ
@stevenlcf

This comment has been minimized.

Copy link

commented Jun 24, 2019

@tbocek @nventuro it seems that go-ethereum's implementation is not internally consistent. Yes, ethapi.EcRecover() requires 27 or 28, but I found some examples in go-ethereum's latest version(v1.8.27) which are using 0 or 1 for V value:

Since the sign functions in go-ethereum still uses 0 or 1 for V, I think it would be better if we could keep both 0/1 and 27/28 for V for now. Otherwise, tx signed by the latest go-ethereum would get reverted by solidity code using the latest open-zeppelin lib.

@nventuro

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

This is very interesting @stevenlcf, thank you for reporting this! From my understanding of the reply you received from the geth team here, we can still count on 27/28 being valid v values, since that's what the high-level interfaces return.

@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@stevenlcf @nventuro. This interesting! I agree with @nventuro that when using high level signers, 27l28 should be used. Its unfortunate that they switched back to 0/1 internally, but I guess they had their reasons. Switching back means being vulnerable to signature malleability attacks for inexperienced Solidity users.

@ukstv

This comment has been minimized.

Copy link

commented Jul 15, 2019

Apparently, web3 1.0 and ethereumjs-util both export 0/1 instead of 27/28. Any ideas on how to handle that, in addition to go-ethereum?

@tbocek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@ukstv web3 returns for getSignatureParameters 27/28. Also ethereumjs-utils adds 27.

What could be done, is to provide a function to canonicalize the signature (if s in upper half order-> s=N-s, v+27 if v<27). Then you can pass your signature to this function and always get a unique signature back. This would also work if you got a signature with 0/1.

@frangio

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

@ukstv Can you share sample code that generates 0/1 signatures?

@ukstv

This comment has been minimized.

Copy link

commented Jul 15, 2019

@ukstv

This comment has been minimized.

Copy link

commented Jul 15, 2019

Ok, sorry. It is ganache’s fixed wrongdoing.

@mrwillis

This comment has been minimized.

Copy link

commented Jul 18, 2019

@ukstv Have you been able to figure out how to get this to work with ganache?. I currently can't get signatures to verify since 2.2

@frangio

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

I believe this will be fixed in the upcoming Ganache 2.6.1.

@mrwillis

This comment has been minimized.

Copy link

commented Jul 18, 2019

@frangio Thanks. Yes just read this on ganache-core

pash7ka added a commit to kittiefight/DutchAuction that referenced this pull request Jul 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.