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

11 stars 10 forks source link

Doesn't Follow ERC1271 Standard #288

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/interfaces/ISignatureValidator.sol#L6 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L19 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Impact

As Per EIP-1271 standard ERC1271_MAGIC_VAULE should be 0x1626ba7e instead of 0x20c13b0b and function name should be isValidSignature(bytes32,bytes) instead of isValidSignature(bytes,bytes). Due to this, signature verifier contract go fallback function and return unexpected value and never return ERC1271_MAGIC_VALUE and always revert execTransaction function.

Proof of Concept

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L6

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L19

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

Tools Used

Manual Review

Recommended Mitigation Steps

Follow EIP-1271 standard.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #370

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed

livingrockrises commented 1 year ago

this should be a seperate primary issue

c4-sponsor commented 1 year ago

livingrockrises requested judge review

c4-judge commented 1 year ago

gzeon-c4 marked the issue as selected for report

gzeon-c4 commented 1 year ago

Selected as best as this issue also mention the wrong function signature.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as satisfactory