Biconomy - Smart Contract Wallet contest - wallstreetvilkas's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L314-L342

Vulnerability details

Impact

Detailed description of the impact of this finding.

It's possible to forge a fake signature and execute a tx from victim's wallet. The function checkSignatures() checks the v. If v is 0, the r value is initialized to a contract address. The attacker can make the signature, so that the v is 0, and r decodes to a malicious contract's address. At the end of the function, the signer contract is called. Because the signer in this case is a malicious contract, once the function call gets sent to the contract, the attacker controlled malicious contract simply returns the magic value. This way all the checks are bypassed, and then it continues the execTransaction() function and the attacker executes the transaction.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L314-L342 This part of the function checkSignatures() is bad.

1. Take the malicious contract address and perform conversion from address -> uint160 -> uint256 -> bytes32 This will be the r. Craft the s. And then finally the v variable. Set it to uint8 0. Craft a fake signature.

2. Create a malicious contract which will return the magic value to pass the check and use it's address in the signature: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

3. Using the fake signature, call execTransaction() with the sig and transaction data. It succeeds.

Tools Used

manual review

Recommended Mitigation Steps

I believe this line https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342 should be changed. It incorrectly assumes that the signer is non malicious and returns a fair value. I'm not really sure what's the solution here, maybe don't call the signer, rather call a trusted contract that actually validates the data.