Ability to bypass the despositFor() function's check and to set the PaymasterId to the address of a smart contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L48-L53 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/libs/LibAddress.sol#L11-L16

Vulnerability details

Impact

The "depositFor()" function verifies that the PaymasterId address is not a smart contract before allowing it to continue and to deposit. However, the extcodesize can be bypassed easily, which allows an attacker to set the PaymasterId address to the address of a smart contract and to deposit with it.

Proof of Concept

It is possible to bypass the "assembly { csize := extcodesize(account) } return csize != 0;" check by calling the depositFor() function from the constructor of a smart contract and to set the PaymasterId's value to the address of the smart contract. By doing this, the extcodesize(account) value will be 0. As such, the isContract() will return false to the depositFor() function and will allow the attacker to bypass the check with a smart contract address.

Recommended Mitigation Steps

Do not trust the result of the extcodesize(account) check, because it can be bypassed easily by using the constructor of a smart contract.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises marked the issue as sponsor acknowledged