The signature validation doesn't verify the chain of trust back to the safe owners. The safe owners signatures are discarded after decompilation. An attacker could generate valid trusted validator signatures without safe owner consent

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/PolicyValidator.sol#L156-L167 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/PolicyValidator.sol#L100-L142

Vulnerability details

Impact

Loss of control and loss of funds for Safe owners. Their signatures are meant to authorize transactions, but the logic discards them and enables potential theft.

Proof of Concept

the signature validation in the provided code does not properly verify the chain of trust back to the safe owners. Here is a detailed explanation: The key part of the vulnerability is in the _decompileSignatures function:

   function _decompileSignatures(bytes calldata _signatures) 
       internal
       pure
       returns (uint32 expiryEpoch, bytes memory validatorSignature)  
   {
     //...

     // Discard safe owners signatures
     validatorSignature = _signatures[length - 8 - sigLength:length - 8];

     // Return only validator signature and expiry 
     return (expiryEpoch, validatorSignature);
   }

This function is discarding the safe owners signatures and only returning the trusted validator's signature. Later in isPolicySignatureValid, it calls _decompileSignatures to extract the validator signature and only verifies that signature:

   (uint32 expiryEpoch, bytes memory validatorSignature) = _decompileSignatures(signatures);

   // Validate only validator signature
   return SignatureCheckerLib.isValidSignatureNow(
     trustedValidator, txnValidityDigest, validatorSignature  
   );

The key issues are:

1. The _decompileSignatures function discards the safeSignature which contains the signatures from the safe owners. It only keeps the validatorSignature from the trusted validator.
2. There is no verification that the validatorSignature corresponds to the discarded safeSignature. An attacker could generate a valid validatorSignature for any transaction without needing the safe owners to sign it.
3. The isPolicySignatureValid function only verifies the validatorSignature against the trusted validator address. It does not verify that the validator signature was authorized by the safe owners.

Here is how an attack would work:

1. Attacker calls _decompileSignatures with a fabricated safeSignature and a real validatorSignature they signed themselves for a malicious transaction.
2. _decompileSignatures discards the fabricated safeSignature and returns the real validatorSignature.
3. isPolicySignatureValid sees a valid validatorSignature and returns true even though the safe owners never signed off on the transaction. This allows the attacker to bypass the safe owner approval process.

Tools Used

Manual

Recommended Mitigation Steps

_decompileSignatures should not discard safeSignature and isPolicySignatureValid should verify it contains valid safe owner signatures that match the discarded validatorSignature. A suggestive example:

   // _decompileSignatures
   function _decompileSignatures(bytes calldata _signatures) 
       internal 
       view 
       returns (uint32 expiryEpoch, bytes memory validatorSignature, bytes memory safeSignatures)  
   {
     //...

     safeSignatures = _signatures[0:length - 8 - sigLength];

     //...
   }

   // isPolicySignatureValid

   // Verify safeSignatures contains valid owner sigs
   require(verifySafeOwnerSigs(safeSignatures, account), "Invalid safe sigs"); 

   // Verify validatorSignature matches discarded safeSignatures
   require(matchesSafeSigs(validatorSignature, safeSignatures), "Invalid validator sig");

This ensures the validator signature can only be considered valid if authorized by the safe owners, preventing the attack.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #44

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

Direct invocation of the PolicyValidator::isPolicySignatureValid function is inconsequential as it is a non-mutating function (view) and thus would not result in any side-effects.

Additionally, the signatures meant for the Gnosis Safe are discarded because they have already been validated by the Gnosis Safe itself; the PolicyValidator::isPolicySignatureValid function is invoked as part of a Gnosis Wallet transaction execution flow which will validate the signatures properly.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid