Revert EIP-2098 support drop in ECDSA.sol

[k06a]

This refactor allows dApp's to optimize calldata gas usage integrating EIP-2098 even after deployment. Specifically, it reduces the gas cost to 136 units by compressing the signature before it's passed into calldata. This compression strategy takes advantage of the gas pricing mechanism, where 1 non-zero byte costs 16 gas units, and each of the 31 padding zero bytes incurs a cost of 4 gas units.

[changeset-bot]

⚠️ No Changeset found

Latest commit: b76db82

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Amxx]

Hello @k06a

First, a bit of history:

The logic that you propose we introduce was part of the original PR #2582 that added support for short signatures in the library.

This feature was released in 4.1.0 and then removed in patch 4.7.3.

The reason for that removal is simple: some (IMO poorly written) contracts use the signature itself (or its hash) as an identifier. When the signature is consumed, they mark this identifier as consumed.

mapping(bytes => bool) private _used;

function doSomethingWithSignature(..., bytes calldata signature) public {
    require(!_used[signature], "signature already used");
    _used[signature] = true;

    require(hasRole(SOME_ROLE, ECDSA.recover(keccak256(abi.encode(......)), signature), "unauthorized");
    
    // ... do stuff ...
}

Supporting both types of signature means that an attacker is able to take one version of signature that was used, convert it from short to long, or from long to short, and replay the signature. This is terrible contract design, but I get why people expect our library to protected them against signature malleability here.

---

Now, what do we do?

I'd love to see short signature more widelly used, and I'd love to reintroduce native bytes support for short signature. But if we do that this bug is going to happen again. We'll need a lot of warning and documentation. We should have do that in 5.0, using the opportunity of the breaking change to change this assumption about signature uniqueness.

In 4.7.3 we went with security over features. I don't think that is ever a bad choice. Have things changed?

@ernestognw @frangio

[frangio]

The problem was that we had promised non-malleability, and implemented non-malleability measures, which completely broke when transparent support for EIP-2098 was introduced.

I agree that I'd like to see this signature format used more widely.

I don't see any issue with adding malleability as long as we do it in a new function so that we don't break the promises of the existing function(s). We would also remove the other non-malleability measures in this new function because they become irrelevant. Ideally, there would be a documentation note about how to avoid the pitfalls of malleability in a contract (i.e., using a nonce).

Have things changed?

I would argue that nothing in the environment has changed. EIP-2098 was always preferable. What would be different this time is that adding malleability would be a deliberate decision.

[k06a]

Thanks for the clarification @Amxx @frangio. Sad but true.