code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L302-L353

Vulnerability details

Impact

checkSignatures is a public function, so that it could be used by external callers to verify a signature. However, the function checks if the signature is valid for the passed dataHash but never checks if the dataHash passed is valid for the data passed. If a caller passes an invalid data, a valid dataHash and a valid signature, then the function will still return true which is incorrect. It's worth mentioning that checkSignatures is used in execTransaction, but there is no impact on it since execTransaction is calculating the dataHash of the data rather than receiving it as a parameter.

Proof of Concept

Check checkSignatures (line 342) in SmartAccount.sol, data parameter is only used when it is a contract signature.

Tools Used

Manual analysis

Recommended Mitigation Steps

Hash the data and compare the result with the passed dataHash

For example,


bytes32 calculatedDataHash = calcDataHash(data); // assuming calcDataHash calculates the hash of passed data
require(calculatedDataHash == dataHash, "Invalid dataHash");

c4-sponsor commented 1 year ago

livingrockrises marked the issue as disagree with severity

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)