Brahma - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleFallbackHandler.sol#L44

Vulnerability details

Proof of Concept

Each subaccount should have policy that was provided for them by main console. So every time, when subaccount owners want to execute any transaction, then SafeModerator will check tx. It will validate if tx doesn't violate it's policy. So PolicyValidator.isPolicySignatureValid function will be called to do that check.

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/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);
    }

What this function does is actually the check that offchain trusted validator indeed has allowed(signed) subaccount to execute such transaction. Also it checks that this approve is not old: expiryEpoch < uint32(block.timestamp), which means that validator will sign messages for some period of time. When signature is old, then transaction can't be executed.

For each subaccount ConsoleFallbackHandler is set as fallback at the deployment. It has isValidSignature function in order to implement EIP-1271.

Let's check how it works. The function calls another version of isValidSignature function. https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleFallbackHandler.sol#L39-L55

    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        GnosisSafe safe = GnosisSafe(payable(msg.sender));
        bytes32 messageHash = getMessageHashForSafe(safe, _data);
        if (_signature.length == 0) {
            require(safe.signedMessages(messageHash) != 0, "Hash not approved");
        } else {
            /// @dev For validating signatures, `PolicyValidator` is invoked to ensure that
            /// the intent of `messageHash` signed by the owners of the safe, complies with its committed
            /// policy
            PolicyValidator policyValidator =
                PolicyValidator(AddressProviderService._getAuthorizedAddress(_POLICY_VALIDATOR_HASH));
            require(policyValidator.isPolicySignatureValid(msg.sender, messageHash, _signature), "Policy not approved");
            safe.checkSignatures(messageHash, _data, _signature);
        }
        return EIP1271_MAGIC_VALUE;
    }

So the purpose of this function is to check if signature is valid. How it can be used? For example subaccount is an owner in another Safe wallet, so on signature validation of tx from that wallet, isValidSignature function of subaccount will be called. In this case, subaccount also should only be able to provide signatures for txs that doesn't violate their's policy. That's why ConsoleFallbackHandler.isValidSignature has policy validation again. So ConsoleFallbackHandler expects, that when signatures will be checked, then it should pass policy validation at the moment of check.

But there is another way to make signature valid. This way is to use SignMessageLib contract and store your message into the signedMessages. If such message is stored then there will be no check in the isValidSignature function.

Such ability allows owners of subaccount to bypass validation sometimes. For example, as i have described before each validator signature has expiring timestamp, which means that signature should be valid only some time. But in case if owner will store this message to signedMessages then on the time of execution will be no policy validation and expiration will not be checked.

Another thing that can happen is that policy will be changed for the subaccount. And after that all messages stored in the signedMessages will be still valid.

I don't know exactly how validator will work, but it's possible for owners of subaccount to store some messages into signedMessages that they would like to be used by other Safe wallets just before their policy will be chnaged. As result all those signatures will work against their policy.

Impact

Subaccounts may provide signatures that violate their policy.

Tools Used

VsCode

Recommended Mitigation Steps

If think about expiration time check, then i would remove signedMessages from isValidSignature validation. So it's possible only to use fresh signatures. If think about changed policy for subaccount only, then i would remove all signedMessages from subaccount, when policy is changed. So this move can be executed as multi send from console safe: first remove all signedMessages and then change policy for subaccount.

Assessed type

Error