search

code-423n4 / 2021-08-realitycards-findings

1 stars 0 forks source link

Direct usage of `ecrecover` allows signature malleability #63

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

shw

Vulnerability details

Impact

The verify function of NativeMetaTransaction calls the Solidity ecrecover function directly to verify the given signature. However, the ecrecover EVM opcode allows for malleable (non-unique) signatures and thus is susceptible to replay attacks. Although a replay attack on this contract is not possible since each user's nonce is used only once, rejecting malleable signatures is considered a best practice.

Proof of Concept

Referenced code: NativeMetaTransaction.sol#L97

SWC-117: Signature Malleability SWC-121: Missing Protection against Signature Replay Attacks

Recommended Mitigation Steps

Use the recover function from OpenZeppelin's ECDSA library for signature verification.

Splidge commented 3 years ago

This is a duplicate of an issue found in the last contest.

It was not solved last time because we didn't want to risk introducing problems by completely rewriting our own version of the MetaTx contract (the version we are using is from a trusted source and working in many production environments that have been audited), so instead of writing our own version I attempted to use the OpenZeppelin version. We need to use the upgradable versions of the OpenZeppelin contracts because we clone RCMarket.sol, however there was an incompatibility between ERC2771Context.sol which uses an immutable variable in the constructor and AccessControl.sol which calls _msgSender() in the constructor. Details on this can be found in this thread..

As you can see in that thread the incompatibility has apparently been fixed, however this was done only 5 hours before the start of this contest (and after we had submitted the code for the contest), so it wasn't practical to implement this solution for this contest. Regardless as you say this attack is "not possible since each user's nonce is used only once".

I'll mark this as 'disputed' for now, if the judges decide that we should have re-written our own version (or discovered time-travel and implemented the OpenZeppelin version that's now availiable) then I'd be happy to change it to 'acknowledged' as we will not be making changes to such a sensitive area at this stage in the project.

0xean commented 3 years ago

Regardless of the solution and the mitigation steps the team is willing or not willing to take, I think highlighting the malleability issue is a valid 1 (Low Risk) issue that may not be worth fixing based on other risks as mentioned by @Splidge