search

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

8 stars 7 forks source link

`PolicyValidator.sol` would fail to validate signatures from counterfactual wallets #239

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#L100-L142

Vulnerability details

Impact

This vulnerability significantly impairs Brahma's ability to validate signatures for counterfactual ERC-4337 accounts, thereby constraining the functionality for users of certain wallets that depend on AA.

Background

EIP 1271 is employed within Brahma. This enables contracts to sign messages and harmonizes perfectly with EIP 4337 (account abstraction). Multiple utilization is evident in scope including ConsoleFallbackHandler.sol.

Note that for ERC-4337, the account isn't deployed until the inaugural UserOp is mined. Until then, the account exists "counterfactually" – possessing an address, even though it's undeployed. This typically benefits the user, allowing the counterfactual address to receive assets without an initial deployment, which means that in this instance if a check is made for this account on if it's a contract or not, the test would always assume it's not a smart contract.

Also, the inability to sign messages from counterfactual contracts/accounts has consistently been a limitation of ERC-1271, exarcebating the issue in this case is the fact current implementation of verifying signatures would not even query the isValidSignature() function for these counterfactual accounts and insteadECDSA.tryrecover() would be done which would lead to not being able to correctly validate the signatures

Proof of Concept

Take a look at PolicyValidator.sol#L100-L142

    function isPolicySignatureValid(address account, bytes32 transactionStructHash, bytes calldata signatures)
        public
        view
        returns (bool)
    {
        // Get policy hash from registry
        bytes32 policyHash =
            PolicyRegistry(AddressProviderService._getRegistry(_POLICY_REGISTRY_HASH)).commitments(account);
        if (policyHash == bytes32(0)) {
            revert NoPolicyCommit();
        }

        // Get expiry epoch and validator signature from signatures
        (uint32 expiryEpoch, bytes memory validatorSignature) = _decompileSignatures(signatures);

        // Ensure transaction has not expired
        if (expiryEpoch < uint32(block.timestamp)) {
            revert TxnExpired(expiryEpoch);
        }

        // Build validation struct hash
        bytes32 validationStructHash = TypeHashHelper._buildValidationStructHash(
            TypeHashHelper.Validation({
                transactionStructHash: transactionStructHash,
                policyHash: policyHash,
                expiryEpoch: expiryEpoch
            })
        );

        // Build EIP712 digest with validation struct hash
        bytes32 txnValidityDigest = _hashTypedData(validationStructHash);

        address trustedValidator = AddressProviderService._getAuthorizedAddress(_TRUSTED_VALIDATOR_HASH);

        // Empty Signature check for EOA signer
        if (trustedValidator.code.length == 0 && validatorSignature.length == 0) {
            // TrustedValidator is an EOA and no trustedValidator signature is provided
            revert InvalidSignature();
        }

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

As seen after a few checks, the function queries the SignatureCheckerLib library's isValidSignatureNow() , navigating to this function, we can alson see that a query is first made to check if the signer is a contract or not. If they are a contract the EIP1271 implemented isValidSignature() would be used otherwise ECDSA.tryRecover would be done, which would mean that at the time this function is queried for counterfactual wallets the route of ECDSA.tryRecover would be used instead since the contract is still not being deployed.

Using the fact that protocol massively integrates with Safe, I assume it's decided to support contract-based wallets (that support ERC4337) and implement ERC1271, my believe is that they "inherit" the possibility from the wallet providers to support ERC4337 too.

This issue may seem like a problem with ERC4337 or EIP1271. But the heart of it is the fact that protocol is taking responsibility to check the validity of signatures, but partially failing to trigger signature validation signatures for a group of wallets from a provider (the validation will succeed if the ERC4337 wallet is deployed).

Tool used

Manual Review

Recommended Mitigation Steps

EIP-6492 provides a solution. The EIP's author has already incorporated ERC-6492 into a universal library which verifies 6492, 712, and 1271 signatures via a specific pull request.

ERC-6492 presumes that the signing contract is typically a contract wallet. However, it could be any contract that adopts ERC-1271 and is counterfactually deployed.

Brahma could incorporate the reference-implementation as presented in the EIP. This would transfer the responsibility of supporting counterfactual signatures back to the wallets. This is viable since, according to the EIP, the wallet should consistently return the magic value.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #46

c4-pre-sort commented 1 year ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #267

raymondfam commented 1 year ago

Could be linked to #46 too.

c4-judge commented 1 year ago

alex-ppg marked the issue as unsatisfactory: Invalid