code-423n4 / 2023-01-biconomy-findings

7 stars 9 forks source link

Cross-Chain Signature Replay Attack #466

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L77-L90

Vulnerability details

Cross-Chain Signature Replay Attack

Impact

User operations can be replayed on smart accounts accross different chains. This can lead to user's loosing funds or any unexpected behaviour that transaction replay attacks usually lead to.

Proof of Concept

As specified by the EIP4337 standard to prevent replay attacks ... the signature should depend on chainid. In VerifyingSingletonPaymaster.sol#getHash the chainId is missing which means that the same UserOperation can be replayed on a different chain for the same smart contract account if the verifyingSigner is the same (and most likely this will be the case).

Tools Used

Manual review

Recommended Mitigation Steps

Add the chainId in the calculation of the UserOperation hash in VerifyingSingletonPaymaster.sol#getHash

    function getHash(UserOperation calldata userOp)
    public view returns (bytes32) { // @audit change to view
        //can't use userOp.hash(), since it contains also the paymasterAndData itself.
        return keccak256(abi.encode(
                userOp.getSender(),
                userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.callGasLimit,
                userOp.verificationGasLimit,
                userOp.preVerificationGas,
                userOp.maxFeePerGas,
                userOp.maxPriorityFeePerGas
        block.chainid // @audit add chain id
            ));
    }
gzeoneth commented 1 year ago

might consider as dupe of #469

c4-judge commented 1 year ago

gzeon-c4 marked the issue as primary issue

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed

c4-judge commented 1 year ago

gzeon-c4 changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

gzeon-c4 marked the issue as selected for report

vladbochok commented 1 year ago

@gzeoneth @livingrockrises

User operations can be replayed on smart accounts accross different chains

The author refers that the operation may be replayed on a different chain. That is not true. The "getHash" function derives the hash of UserOp specifically for the paymaster's internal usage. While the paymaster doesn't sign the chainId, the UserOp may not be relayed on a different chain. So, the only paymaster may get hurt. In all other respects, the bug is valid.

The real use case of this cross-chan replayability is described in issue #504 (which, I believe, was mistakenly downgraded).

livingrockrises commented 1 year ago

true. besides chainId , address(this) should be hashed and contract must maintain it's own nonces per wallet otherwise wallet can replay the signature and use paymaster to sponsor! we're also planning to hash paymasterId as add-on on top of our off-chain validation for it.

I have't seen an issue which covers all above. either cross chain replay or suggested paymaster nonce