Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross-Chain Signature Replay Attack #466

Open
code423n4 opened this issue Jan 9, 2023 · 7 comments
Open

Cross-Chain Signature Replay Attack #466

code423n4 opened this issue Jan 9, 2023 · 7 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue M-03 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

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
            ));
    }
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 9, 2023
code423n4 added a commit that referenced this issue Jan 9, 2023
@gzeoneth
Copy link
Member

might consider as dupe of #469

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Jan 17, 2023
@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 7, 2023
@c4-sponsor
Copy link

livingrockrises marked the issue as sponsor confirmed

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Feb 10, 2023
@c4-judge
Copy link
Contributor

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

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Feb 10, 2023
@vladbochok
Copy link

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue M-03 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

7 participants