Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage of floating pragma #17

Open
hats-bug-reporter bot opened this issue Jun 19, 2024 · 5 comments · Fixed by safe-global/safe-modules#458
Open

Usage of floating pragma #17

hats-bug-reporter bot opened this issue Jun 19, 2024 · 5 comments · Fixed by safe-global/safe-modules#458
Labels
bug Something isn't working Lead - low low

Comments

@hats-bug-reporter
Copy link

Github username: @@giorgiodalla
Twitter username: 0xAuditism
Submission hash (on-chain): 0xf8de327cbd43eb23fcf2c5b769f076dc475ce3cae496ff82150905b23006f7a1
Severity: low

Description:
Description
All contracts in scope have flaoting pragma.
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Note that libraries can still be used with floating pragmas.

Attack Scenario
Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File
@> pragma solidity >=0.8.0;

import {SignatureValidator} from "../base/SignatureValidator.sol";
import {ISafe} from "../interfaces/ISafe.sol";
import {P256, WebAuthn} from "../libraries/WebAuthn.sol";

/**
 * @title Safe WebAuthn Shared Signer
 * @dev A contract for verifying WebAuthn signatures shared by all Safe accounts. This contract uses
 * storage from the Safe account itself for full ERC-4337 compatibility.
 */
contract SafeWebAuthnSharedSigner is SignatureValidator {
  1. Revised Code File (Optional)
    Consider adding fixed pragmas, this can be done such as:
-pragma solidity >=0.8.0;
+pragma solidity >=0.8.0;
import {SignatureValidator} from "../base/SignatureValidator.sol";
import {ISafe} from "../interfaces/ISafe.sol";
import {P256, WebAuthn} from "../libraries/WebAuthn.sol";

/**
 * @title Safe WebAuthn Shared Signer
 * @dev A contract for verifying WebAuthn signatures shared by all Safe accounts. This contract uses
 * storage from the Safe account itself for full ERC-4337 compatibility.
 */
contract SafeWebAuthnSharedSigner is SignatureValidator {
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Jun 19, 2024
@nlordell
Copy link
Collaborator

nlordell commented Jun 20, 2024

Thank you, while I do believe there is merit to the proposal, I don't believe that there is a clear need for using fixed Solidity contract pragmas. In fact we don't use them in the Safe core contracts. There are trade-offs to both approaches. Furthermore, the project includes documentation about the exact compiler version that is used, and verified code explorers like Sourcify and Etherscan clearly show the Solidity version the contract was compiled with.

I personally see this as a "coding style" suggestion which I don't think is in scope for the audit competition and as such will mark it as invalid.

@nlordell nlordell added the invalid This doesn't seem right label Jun 20, 2024
@auditism
Copy link

I respect your choice and thank you for giving such a detailed explanation, I do appreciate that.
The reason I did submit this low issue is because all contracts besides one had a floating pragma. So I thought that it was an oversight on your part, since if it was a coding style choice there would have been consistency.
Anyways it is true that the impact is non existent and is considered a best practice, but in most audit contest it is still a valid low.

@nlordell nlordell removed the invalid This doesn't seem right label Jun 20, 2024
@nlordell
Copy link
Collaborator

Thank you for your reply.

I agree with you that the pragmas are inconsistent. In fact some contracts use ranges, others use ^ and others use unbounded ranges.

The decision to use floating pragmas was also re-evaluated by our team and we decided to implement your suggestion (as well as make other pragmas consistent throughout the code - making sure that the minimum version is indeed the minimum version that the contracts build with).

As such, we have reconsidered and will be awarding this as a "low" find. Thanks for your submission!

@0xEricTee
Copy link
Collaborator

@nlordell Reviewed the fix and the amendment looks fine, thanks!

nlordell added a commit to safe-global/safe-modules that referenced this issue Jul 5, 2024
Fixes
hats-finance#17

This PR changes the pragmas in the passkeys contract to be:
1. More consistent: we use `^0.8.20` everywhere for "non-top-level"
contracts. The version was chosen as this it the first version that
supports all of the compiler features that we make use of. Notably, we
have `@custom:` NatSpec items which are only officially supported as of
v0.8.20 (incidentally, this is the same version used by OpenZeppelin
contracts for similar reason:
[source](OpenZeppelin/openzeppelin-contracts#4489)).
The choice to use floating pragmas here is to make using these support
contracts easier across other projects (namely, the `WebAuthn` and
`P256` libraries are useful outside of this project). Furthermore, we
use `^` versions so that breaking changes introduced in a potential
Solidity v0.9 would not affect the security and integrity of the
contract code.
2. Use fixed pragmas for "top-level" contracts that we deploy:
    - `SafeWebAuthnProxyFactory`
    - `SafeWebAuthnSharedSigner`
    - `FCLP256Verifier`

I am aware that Solidity v0.8.20 doesn't play nice with chains like BNB
by default, however this can be worked around by explicitly by setting
the EVM version target (as we do in this project) and do not believe
this is an issue.
@fonstack fonstack reopened this Jul 9, 2024
@nlordell
Copy link
Collaborator

nlordell commented Jul 9, 2024

Oops, looks like GitHub automatically closed the issue - sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Lead - low low
Projects
None yet
4 participants