Signature malleability in isPolicySignatureValid() and executeTransaction()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ExecutorPlugin.sol#L140-L140
https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/PolicyValidator.sol#L141

Vulnerability details

Impact

Both isPolicySignatureValid() and executeTransaction() are vulnerable to signature malleability, which allows replay attacks.
Those functions use solady SignatureCheckerLib.isValidSignatureNow to verify signature. According to solady codebase: "This implementation does NOT check if a signature is non-malleable.".

During the previous Code4rena contest, the similar attack vector has been evaluated as Medium/High

• [Medium] cancelSig will not completely cancel signatures due to malleability vulnerabilities 2023-07-nounsdao-findings#198
• [High] https://code4rena.com/reports/2021-04-meebits#h-01-signature-malleability-of-evms-ecrecover-in-verify

Based on that, I've decided to evaluate the risk of this issue as Medium. It's worth to note, that in most of the previous Code4rena contests, the signature malleability had been detected during the bot race, so it was not reported as a separate issue. Thus, there aren't many contests which could be used as a reference for proper severity categorization. In the current contest - the bot race did not report signature malleability issue, so I'm reporting it as a separate issue in this report.

Proof of Concept

• In src/core/PolicyValidator.sol, there is a function isPolicySignatureValid() responsible for validating signature against policies for module execution.
  We can see, that it uses SignatureCheckerLib.isValidSignatureNow to perform signature validation:

File: src/core/PolicyValidator.sol

 return SignatureCheckerLib.isValidSignatureNow(trustedValidator, txnValidityDigest, validatorSignature);

In src/core/ExecutorPlugin.sol, there's a function executeTransaction, which enables executors to raise execution requests that will be executed via a module transaction. That function, calls internal _validateExecutionRequest, which is internal helper to validate the execution request.
We can see, that it uses SignatureCheckerLib.isValidSignatureNow to validate executor signature:

File: src/core/ExecutorPlugin.so

    function executeTransaction(ExecutionRequest calldata execRequest) external nonReentrant returns (bytes memory) {
        _validateExecutionRequest(execRequest);

File: src/core/ExecutorPlugin.so

if (
            !SignatureCheckerLib.isValidSignatureNow(
                execRequest.executor, _transactionDigest, execRequest.executorSignature
            )
        ) {
            revert InvalidExecutor();
        }

Let's take a closer look at SignatureCheckerLib.isValidSignatureNow now.

In both files, it's imported from solady/utils/SignatureCheckerLib.sol. Let's check the solady implementation then:

source: https://github.com/Vectorized/solady/blob/main/src/utils/SignatureCheckerLib.sol

/// WARNING! Do NOT use signatures as unique identifiers.
/// Please use EIP712 with a nonce included in the digest to prevent replay attacks.
/// This implementation does NOT check if a signature is non-malleable.

As stated in the solady comment section, the library does not provide any protection from signature malleability. This can be additionally confirmed by looking at the implementation of isValidSignatureNow().

This implies, that since isPolicySignatureValid() and executeTransaction() uses solady implementation - there are suspicable to signature malleability

Tools Used

Manual code review

Recommended Mitigation Steps

Consider using OpenZeppelin's ECDSA library: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol

Assessed type

Library

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #212

[c4-pre-sort]

raymondfam marked the issue as duplicate of #398

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden has referenced past issues that are irrelevant to the present submission; the signature malleability that the Solady library talks about is in relation to a known flaw in the ecrecover precompile; it will not validate whether a particular s value in an r, s, and v signature payload can be replayed.

The ECDSA curve utilized in Ethereum is symmetric on the X axis, meaning that a particular point in the curve always contains a valid opposite on the other side of the X axis. In ecrecover, this manifests as a "vulnerability" as two different s values can validate the same signed payload.

To prevent this misbehaviour, the official recommendation is to restrict X axis values (s) in the lower-half order of the secp256k1n curve, meaning that s should be less-than-or-equal-to 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0. This is taken care of by libraries such as OpenZeppelin's ECDSA library but Solady deliberately avoids validating this as a matter of optimization.

In this particular instance within the Brahma project, the vulnerability is inconsequential as a replay attack cannot occur; the PolicyValidator which will validate the relevant executor request will utilize the nonce of the Gnosis Wallet which will be incremented after the transaction succeeds. As such, this exhibit is invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid