Skip to content

Stack too deep error on safe-smart-account/contracts/Safe.sol #2

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

Closed
bugduino opened this issue Apr 17, 2025 · 13 comments
Closed

Stack too deep error on safe-smart-account/contracts/Safe.sol #2

bugduino opened this issue Apr 17, 2025 · 13 comments

Comments

@bugduino
Copy link

Hi,
I'm trying to use the library as specified in the Readme but when I try to compile the script I get a Stack too deep error on
lib/safe-utils/lib/safe-smart-account/contracts/Safe.sol:33:1

Image

Is there anything that should be done maybe not documented?
I'm using solidity v0.8.29 and already using via_ir in foundry.toml.
The safe-smart-account lib is at this commit bf943f80fec5ac647159d26161446ac5d716a294 (HEAD, tag: v1.4.1)

@aviggiano
Copy link
Collaborator

Hey

Thanks for trying this out. I'll take a look, but meanwhile, are you using optimization? I have runs = 200 on my project and it's working fine

@aviggiano
Copy link
Collaborator

@bugduino can you share your script? I can take a look

@bugduino
Copy link
Author

The repo is private unfortunately but I can share the upgrade script alone if that is useful although it does not compile given the issue above so not sure if it's really needed.

Btw in foundry.toml I tried both with no optimization and with 200 runs but I got the same result

This is my foundry.toml default profile

[profile.default]
  auto_detect_solc = false
  block_timestamp = 1_738_368_000 # Feb 1, 2025 at 00:00 GMT
  bytecode_hash = "none"
  evm_version = "shanghai"
  fuzz = { runs = 1_000 }
  gas_reports = ["*"]
  optimizer = true
  optimizer_runs = 10_000
  via_ir = true
  out = "out"
  script = "script"
  solc = "0.8.29"
  src = "src"
  test = "tests"
  ffi = true
  ast = true
  build_info = true
  extra_output = ["storageLayout"]
  fs_permissions = [{ access = "read", path = "./out/"}]

@bugduino
Copy link
Author

This is the simplified script I'm using

pragma solidity >=0.8.28 <0.9.0;

import { Contract } from "../src/Contract.sol";
import { Script } from "forge-std/Script.sol";
import { console } from "forge-std/console.sol";
import { Upgrades, Options } from "@openzeppelin/foundry-upgrades/Upgrades.sol";
import { Safe } from "safe-utils/Safe.sol";
import { IProxyAdmin } from "@openzeppelin/foundry-upgrades/internal/interfaces/IProxyAdmin.sol";

contract DeployScript is Script {
  using Safe for *;
  error Invalid();

  string public constant BUILD_INFO_DIR = "old-build-info/";

  // Gnosis Safe instance
  Safe.Client safe;

  // Optimism test contracts
  string public constant network = "optimism";
  address public constant SAFE_ADDRESS = address(2); // CHANGE THIS
  address public constant PROXY = address(1); // CHANGE THIS 

  function run() public {
    vm.createSelectFork(network);
    vm.startBroadcast();
    console.log('Upgrading in', network);
    _upgradeContractMultisig();
    vm.stopBroadcast();
  }

  // Methods to propose an upgrade to the proxy implementation
  function _upgradeContractMultisig() internal {
    _upgradeViaMultisig(PROXY, "Contract");
  }
  
  /// @notice Prepares the upgrade of the proxy implementation to a new version.
  /// @param proxy The address of the proxy contract to upgrade.
  /// @param oldContract The name of the old contract to upgrade.
  function _upgradeViaMultisig(address proxy, string memory oldContract) public {
    if (proxy == address(0) || bytes(oldContract).length == 0) {
      revert Invalid();
    }
    safe.initialize(SAFE_ADDRESS);

    // https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades?tab=readme-ov-file#upgrade-a-proxy-or-beacon
    Options memory opts;
    opts.referenceBuildInfoDir = string(abi.encodePacked(BUILD_INFO_DIR, network));
    opts.referenceContract = string(abi.encodePacked(network, ":", oldContract));
    string memory contractFile = string(abi.encodePacked(oldContract, ".sol"));

    // Validate and deploy new implementation
    address newImpl = Upgrades.prepareUpgrade(contractFile, opts);
    // Propose upgrade tx with multisig
    safe.proposeTransaction(
      Upgrades.getAdminAddress(proxy), 
      abi.encodeCall(IProxyAdmin.upgradeAndCall, (proxy, newImpl, "")), 
      DEPLOYER
    );
  }
}


@bugduino
Copy link
Author

btw If I use only via_ir with optimizer = false the stack is only 1 variable too deep instead of 5

Image

@aviggiano
Copy link
Collaborator

Thanks. Your script looks perfect, so this might be a bottleneck on the library.

I might need to put all arguments from proposeTransaction into a single struct to avoid these kinds of issues. I'll try to work on that over the following days.

@bugduino
Copy link
Author

Thanks! Although the error seems to be referencing the underlying safe-smart-account library rather than safe-utils so not sure the issue is on your side of the code but I may be wrong ofc

@aviggiano
Copy link
Collaborator

@bugduino Can you try v0.0.9?

@bugduino
Copy link
Author

I quickly tried to compile using the latest version and seems to be working now! I'm in the middle of an audit so I unable to test the full upgrade but will do as soon as possible and let you know if everything goes well. In the meantime thanks for the quick fix!

@bugduino
Copy link
Author

@aviggiano when calling the safe.proposeTransaction on the script above using an unlocked ledger (which is one of the signers of the multisig) I get the following error

script failed: vm.sign: operation `sign_hash` is not supported by the signer

@aviggiano
Copy link
Collaborator

So you need to pass the derivation path like this

safe.getExecTransactionData(weth, abi.encodeCall(IWETH.withdraw, (0)), foundrySigner1, "m/44'/60'/0'/0/0");

This will use cast to sign with EIP712, until vm.signTypedData is supported by foundry foundry-rs/foundry#10330

@aviggiano
Copy link
Collaborator

I'll update the README to make it clearer

@bugduino
Copy link
Author

Makes sense. I have another problem but I think it's unrelated to the stack too deep error so I will open another issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants