EVM's `ecrecover` is susceptible to signature malleability

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x45c350f639294143f07190bbce288c6f36fbac4941a74a2e52d8c19db5e88ec1 Severity: medium

Description:

Impact

Potential replay attacks, attacker could forge ecrecover return value to match guardian signatures

Description

The attacker could flip s and v values to create a different signature that equals to the same hash & signer. There are three instances of ecrecover in the codebase:

src/apps/mock/IncentivizedMockEscrow.sol

53:     address messageSigner = ecrecover(keccak256(_message), v, r, s);

src/apps/wormhole/external/callworm/WormholeVerifier.sol

102:    address signatory = ecrecover(hash, v, r, s);

src/apps/wormhole/external/wormhole/Messages.sol

116:    address signatory = ecrecover(hash, sig.v, sig.r, sig.s);

Recommendation

Instead of the vulnerable ecrecover consider using Openzeppelin's ECDSA library: openzeppelin-contracts/utils/cryptography/ECDSA.sol

[reednaa]

src/apps/mock/IncentivizedMockEscrow.sol What is the impact?

src/apps/wormhole/external/callworm/WormholeVerifier.sol Based on Wormhole. What is the impact?

src/apps/wormhole/external/wormhole/Messages.sol Not in scope.

[0xfuje]

Apologies for the late answer. I could finally took some time to explore the issue further.

---

src/apps/mock/IncentivizedMockEscrow.sol

During processPacket() -> verifyPacket() will return true with the maliciously crafted v and s values: however _handleMessage() and _handleAck() will both revert later since checks prevent processing the message twice.

---

src/apps/wormhole/external/callworm/WormholeVerifier.sol

I don't exactly know the full extent of how the contract is going to be used, but every guardian signature validation can be replayed and bypassed by manipulating v and s values. If there are no additional checks in the app this can be exploited, however afaik here it's only used by processPacket() which is not vulnerable for replays (but _verifyPacket() will still execute with the forged values!).

---

Currently there is no impact with processPacket() calls, however it would be dangerous to leave this alone as the contracts still allow signature malleability. I would recommend to switch to Openzeppelin's ECDSA instead of ecrecover. The crucial check OZ makes is to not allow the s value to be in the upper range: link since almost all libraries generate s in the lower half. If you don't want to use OZ i would implement the check instead.

[reednaa]

Considering that there is no impact, I will mark this as won't fix.

Our mock contract is meant to be simple and I personally prefer using ecrecover directly over using an external library.

For Wormhole, the WormholeVerifier.sol is meant to implement Wormhole verification very closely. Considering that could be a lot of signatures verified for Wormhole VAAs, I very much prefer a lean implementation.