`_hasNoCode()`may behave unexpectedly

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x9aa4d38107928431799c3a6ce303e28f66238fd335f517b1b182b77d9c0e96f9 Severity: low

Description: Description\ _hasNoCode() in SafeWebAuthnSignerFactory.solfunction is used to determine whether the account is a contract or an externally owned account (EOA). However, in Solidity, there is no reliable way to definitively determine whether a given address is a contract, as there are several edge cases in which the underlying extcodesize function can return unexpected results.

    function _hasNoCode(address account) internal view returns (bool result) {
        // solhint-disable-next-line no-inline-assembly
        assembly ("memory-safe") {
@>          result := iszero(extcodesize(account))
        }
    }

if an address contains code, it’s not an EOA but a contract account. However, a contract does not have source code available during construction. This means that while the constructor is running, it can make calls to other contracts, but extcodesize for its address returns zero.

Further, such _hasNoCode() was also used by Openzeppelin named as isContract() function in their contracts but Openzeppelin has removed isContract() from utility contracts like Address.sol citing potential misuse.

Link for reference- https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3417

Solidity 0.8.1 implements a shortcut for account.code.length that avoids copying code to memory. Therefore the above code should be equivalent.

Recommendations Consider below changes:

    function _hasNoCode(address account) internal view returns (bool result) {
-        // solhint-disable-next-line no-inline-assembly
-        assembly ("memory-safe") {
-            result := iszero(extcodesize(account))
-        }
+        result = account.code.length > 0;
    }

[0xRizwan]

correction: Contract is ensuring the wallat address has no code means NOT contract address so corrected recommendation wouldn be account.code.length == 0.

[nlordell]

Thank you for the submission! TIL about account.code.length which would be slightly nicer than what we have.

Note that we do not copy code into memory (extcodesize opcode does not copy code) and your suggestion and what is in the contracts are functionally equivalent.

However, code style is not in scope for the audit competition, so I would mark this as invalid.

---

Regarding your other claim:

_hasNoCode() in SafeWebAuthnSignerFactory.sol function is used to determine whether the account is a contract or an externally owned account (EOA). However, in Solidity, there is no reliable way to definitively determine whether a given address is a contract, as there are several edge cases in which the underlying extcodesize function can return unexpected results.

While it is true that there isn't a reliable way to determine whether or not an address has code, I believe that these concerns do not apply in our case:

• The _hasNoCode is only ever called on addresses that are known CREATE2 deployment targets for SafeWebAuthnSignerProxy instances
• It is used to check whether or a CREATE2 deployment should be done or not, and not to see if the account has is an EOA (should be reflected in the function name and documentation - it just returns whether or not the address has code or not)
• The SafeWebAuthnSignerProxy is known to not call this function, so the concern about this returning incorrect values is moot.

With that in mind, I do not believe that this check is misused in our contracts and believe this submission to be invalid.

[0xRizwan]

@nlordell Thanks for the detailed comment, agree with you. Now, account.code.length is preferred over extcodesize with less dependency on assembly except for gas saving, etc. For example, openzeppelin uses account.code.length after deprecating the use of extcodesize.

[nlordell]

Note that it appears that account.code.length being equivalent to extcodesize is only from Solidity 0.8.1+ (source) while the contracts has a pragma for 0.8.0+.

Again, I do appreciate the submission, but I don't believe it is in scope for the audit competition.

[0xRizwan]

Yes, correct. I mentioned this point in report.

Solidity 0.8.1 implements a shortcut for account.code.length that avoids copying code to memory. Therefore the above code should be equivalent.

Contracts will be deployed with 0.8.24 as per config so it will work. Anyways, i respect your decision for this issue.

[nlordell]

Contracts will be deployed with 0.8.24 as per config so it will work.

Yes! WebAuthn is a library contract, however, and it can be used outside of the project.

Thank you for your understanding, and generally high quality submissions and participation in this audit competition :)