code-423n4 / 2023-10-brahma-findings

8 stars 7 forks source link

No access controls on `isPolicySignatureValid`; anyone can validate arbitrary signatures. #130

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

No access controls on the main methods like isPolicySignatureValid. These should be restricted to authorized callers like the Gnosis Safe proxy contract. Otherwise, anyone could validate arbitrary signatures.

Proof of Concept

There is a lack of proper access control protections around this critical entry point. It violates the principle of least privilege by leaving it open to any caller. The impact is that it considerably weakens the security guarantees of policy-based approvals. Almost anyone could potentially bypass policies if the validator remains unrestricted.

This is where lack of access control on isPolicySignatureValid occurs: https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/PolicyValidator.sol#L54-L76

function isPolicySignatureValid(
  address account, 
  //...
) 
  external view returns (bool) 
{
  // ...
}

The external view modifier on the function. This makes it accessible to external caller.

isPolicySignatureValid method is the major entry point for validating policy signatures and approving Safe transactions.

Without access controls, any contract or external account can call this method to validate arbitrary signatures.

This enables the following abuse scenarios:

For example, a flash loan contract could call it to circumvent policies for its own purposes without actual Safe owner approval.

Imagine this attack flow:

  1. Eve deploys a malicious contract MaliciousContract with the intent to steal funds from Alice's Gnosis Safe.

  2. Eve calls PolicyValidator.isPolicySignatureValid directly from MaliciousContract with:

    • Crafted Safe address, signatures, and tx data to transfer funds from Alice's Safe to Eve.

    • No actual signatures from Alice's Safe owners.

  3. PolicyValidator performs policy validation on the crafted data since anyone can call it.

  4. Eve mimics the expected parameters and signature format expected by PolicyValidator.

  5. The forged transaction passes policy validation since PolicyValidator assumes it was called by the trusted Gnosis Safe contract.

  6. Funds are stolen from Alice's Safe as PolicyValidator approved the withdrawal.

This demonstrates how an attacker can directly call the PolicyValidator to arbitrarily bypass policies without Gnosis Safe owner approval.

The lack of access control on isPolicySignatureValid enables this attack vector.

Tools Used

Vs

Recommended Mitigation Steps

Restricting the function to only the "whitelist" of approved Safe contracts would prevent unauthorized callers from exploiting it. Proper access control is crucial.

For example:

// Restrict to authorized list
modifier onlyAuthorized() {
  require(isAuthorized(msg.sender), "Not authorized");
  _;
}

function isPolicySignatureValid(
  // ... 
)
  external view onlyAuthorized returns (bool)  
{
  // ...
}

Where isAuthorized checks the caller against a whitelist of approved contracts like the Gnosis Safe proxy.

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

raymondfam commented 1 year ago

Invalid assumptions.

alex-ppg commented 1 year ago

The referenced function is an external view function, meaning that the blockchain state will not be altered if it is invoked.

Additionally, the Warden showcases a poor understanding of the system; the validator can never execute transactions on behalf of a Gnosis Safe.

c4-judge commented 1 year ago

alex-ppg marked the issue as unsatisfactory: Invalid