search

hats-finance / Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac

A collection of modules that can be used with the Safe contract
GNU Lesser General Public License v3.0
0 stars 1 forks source link

Use `require` instead of `assert` #21

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: @mgf15 Twitter username: -- Submission hash (on-chain): 0x5be0f1346b2fd9422447b2e4d8d1665a6599f0a703d5372f1c84d12243f62adc Severity: low

Description: Description\ Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

MGF15 commented 1 week ago

my bad, forget to add the link 

            assert(address(created) == signer);

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/SafeWebAuthnSignerFactory.sol#L56

nlordell commented 1 week ago

Thank you for the submission. The use of assert here is intended and introduced in this commit: https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/commit/213b84f77064766861dbd598575df81f47b6ec3d

According to the Solidity documentation:

Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.

Which qualifies in this case. It would mean that we are incorrectly computing a CREATE2 address internally in the contract and does not depend on external inputs. This assert represents the invariant that the computed contract address always equals the deployment contract address.

For these reasons, I believe this submission to be invalid.