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

Contracts can not be deployed on BNB chain #19

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

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

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

Description: Description\

The inscope safe contracts compiler version is set as following:

SafeWebAuthnSharedSigner to >=0.8.0,

FCLP256Verifier to 0.8.24,

SafeWebAuthnSignerFactory to >=0.8.0,

SafeWebAuthnSignerProxy to >=0.8.0,

SafeWebAuthnSignerSingleton to >=0.8.0

These are either set as floating pragma or fixed version like 0.8.24. There is inconsistency here. However, the safe contracts would be deployed with solidity version 0.8.24. This can be checked in hardhat.config.ts.

  solidity: {
    compilers: [
      {
        version: '0.8.24',

The different supported chains by Safe can be checked here. One of the safe supported chain is BNB chain and this issue is about it.

For BNB chain, the contracts can not be deployed with solidity 0.8.24 as BNB chain does not support solidity 0.8.24.

More info on BNB chain 0.8.20 and above can be checked here.

It should be noted that BNB Chain has been working on implementing support for Solidity 0.8.20 and above, which includes new features and libraries. While not officially the most recent supported version yet, there is ongoing development for compatibility.

Since, the compiler version in script and contracts is hardcoded and therefore it would require to modify it for BNB chain deployment.

Recommendations\ Use the supported solidity version for BNB chain, Preferably 0.8.19 or less as 0.8.24 is not supported.

0xEricTee commented 1 week ago

Known issue, previously reported in Certora audit report: modules/passkey/docs/v0.2.0/audit-report-certora.pdf::I-01.EVM Version Shanghai may not work on other chains due to PUSH0.

0xRizwan commented 1 week ago

Hi @0xEricTee

This issue is not about PUSH0 support.

Its a different issue arising from deployment on BNB chain. The current contracts with 0.8.24 can not be deployed on BNB chain. This issue is not found in Certora audit and issues related to BNB is not mentioned there.

Further the documentation states,

The project uses Solidity compiler version 0.8.24 with 10 million optimizer runs, as we want to optimize for the code execution costs. The EVM version is set to paris because not all our target networks support the opcodes introduced in the Shanghai EVM upgrade

So use of 0.8.24 is intended by protocol team but they are not aware of this issue related to BNB chain and this issue is not made an exception anywhere, so for BNB chain deployment, the compiler version should be 0.8.19

nlordell commented 1 week ago

For the record, we generally do want to have good chain support (including BNB), however, after some further investigation and it appears this is indeed invalid.

For BNB chain, the contracts can not be deployed with solidity 0.8.24 as BNB chain does not support solidity 0.8.24.

I do not think this is correct. The issue you linked specifically describes that the chain does not support PUSH0. The use of Solidity 0.8.19 is recommended as, by default, it does not generate code with PUSH0 instructions.

We explicitely, as documented, target the EVM version paris (which is the default of Solidity v0.8.19) and would produce BNB compatible bytecode.

In fact, I checked and the deployment bytecode does successfully run on BNB:

curl -s https://binance.llamarpc.com -H 'content-type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"eth_call","params":[{"data":"'(jq -r .bytecode build/artifacts/contracts/SafeWebAuthnSignerFactory.sol/SafeWebAuthnSignerFactory.json)'"},"latest"]}'

Produces the following JSON output:

{
  "jsonrpc": "2.0",
  "id": 0,
  "result": "0x608060405234801561001057600080fd5b506004361061004c5760003560e01c80630d2f0489146100515780635a28a1db1461008e578063a541d91a146100b5578063cb48798b146100c8575b600080fd5b61006461005f36600461050d565b61010c565b60405173ffffffffffffffffffffffffffffffffffffffff90911681526020015b60405180910390f35b6100647f000000000000000000000000f4d9599afd90b5038b18e3b551bc21a97ed21c3781565b6100646100c336600461050d565b61027a565b6100db6100d6366004610542565b6103d9565b6040517fffffffff000000000000000000000000000000000000000000000000000000009091168152602001610085565b600061011984848461027a565b9050803b6102735760008060001b7f000000000000000000000000f4d9599afd90b5038b18e3b551bc21a97ed21c37868686604051610157906104d5565b73ffffffffffffffffffffffffffffffffffffffff90941684526020840192909252604083015275ffffffffffffffffffffffffffffffffffffffffffff1660608201526080018190604051809103906000f59050801580156101be573d6000803e3d6000fd5b5090508173ffffffffffffffffffffffffffffffffffffffff168173ffffffffffffffffffffffffffffffffffffffff16146101fc576101fc6105e1565b6040805173ffffffffffffffffffffffffffffffffffffffff841681526020810187905290810185905275ffffffffffffffffffffffffffffffffffffffffffff841660608201527fedab0e1444e0b8a4d0f70e45c55a457b30c12bff4f38fdb7f53b14134036a8609060800160405180910390a1505b9392505050565b6000806040518060200161028d906104d5565b7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe082820381018352601f909101166040819052610323919073ffffffffffffffffffffffffffffffffffffffff7f000000000000000000000000f4d9599afd90b5038b18e3b551bc21a97ed21c3716908890889075ffffffffffffffffffffffffffffffffffffffffffff891690602001610640565b604080517fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe081840301815282825280516020918201207fff00000000000000000000000000000000000000000000000000000000000000828501523060601b7fffffffffffffffffffffffffffffffffffffffff00000000000000000000000016602185015260006035850152605580850191909152825180850390910181526075909301909152815191012095945050505050565b6000807f000000000000000000000000f4d9599afd90b5038b18e3b551bc21a97ed21c37905060008888886040516024016104169392919061066b565b604080517fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe0818403018152918152602080830180517bffffffffffffffffffffffffffffffffffffffffffffffffffffffff167f1626ba7e0000000000000000000000000000000000000000000000000000000017905290516104a09291899189918991016106bf565b604051602081830303815290604052905060206000825160208401855afa156104c95760005192505b50509695505050505050565b6101ee8061070b83390190565b803575ffffffffffffffffffffffffffffffffffffffffffff8116811461050857600080fd5b919050565b60008060006060848603121561052257600080fd5b8335925060208401359150610539604085016104e2565b90509250925092565b60008060008060008060a0878903121561055b57600080fd5b86359550602087013567ffffffffffffffff8082111561057a57600080fd5b818901915089601f83011261058e57600080fd5b81358181111561059d57600080fd5b8a60208285010111156105af57600080fd5b60208301975080965050505060408701359250606087013591506105d5608088016104e2565b90509295509295509295565b7f4e487b7100000000000000000000000000000000000000000000000000000000600052600160045260246000fd5b6000815160005b818110156106315760208185018101518683015201610617565b50600093019283525090919050565b600061064c8288610610565b9586525050602084019290925260408301526060820152608001919050565b83815260406020820152816040820152818360608301376000818301606090810191909152601f9092017fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe016010192915050565b60006106cb8287610610565b9485525050602083019190915260501b7fffffffffffffffffffffffffffffffffffffffffffff0000000000000000000016604082015260560191905056fe61010060405234801561001157600080fd5b506040516101ee3803806101ee83398101604081905261003091610058565b6001600160a01b0390931660805260a09190915260c0526001600160b01b031660e0526100bc565b6000806000806080858703121561006e57600080fd5b84516001600160a01b038116811461008557600080fd5b60208601516040870151606088015192965090945092506001600160b01b03811681146100b157600080fd5b939692955090935050565b60805160a05160c05160e05160ff6100ef60003960006008015260006031015260006059015260006080015260ff6000f3fe608060408190527f00000000000000000000000000000000000000000000000000000000000000003660b681018290527f000000000000000000000000000000000000000000000000000000000000000060a082018190527f00000000000000000000000000000000000000000000000000000000000000008285018190527f00000000000000000000000000000000000000000000000000000000000000009490939192600082376000806056360183885af490503d6000803e8060c3573d6000fd5b503d6000f3fea2646970667358221220ddd9bb059ba7a6497d560ca97aadf4dbf0476f578378554a50d41c6bb654beae64736f6c63430008180033a26469706673582212207db0f17957224e6381d7a75f0fb3bff10545be67f7d5d3b8cc7d49f5ed7121c364736f6c63430008180033"
}

Note that if we were to not target paris, then deployments would fail. With the following diff:

diff --git a/modules/passkey/hardhat.config.ts b/modules/passkey/hardhat.config.ts
index 9a70aa4..b672a43 100644
--- a/modules/passkey/hardhat.config.ts
+++ b/modules/passkey/hardhat.config.ts
@@ -63,7 +63,7 @@ const config: HardhatUserConfig = {
             runs: 10_000_000,
           },
           viaIR: false,
-          evmVersion: 'paris',
+          evmVersion: 'shanghai',
         },
       },
     ],

The same command would return:

{
  "jsonrpc": "2.0",
  "id": 0,
  "error": {
    "code": -32000,
    "message": "invalid opcode: PUSH0"
  }
}
0xRizwan commented 1 week ago

@nlordell You are correct, a bit oversight from my side, would agree with final decision of this issue. Thanks for your comment.

nlordell commented 1 week ago

Thank you for the thoughtful and high quality submission 🙌