Add a Safe Guard that implements a timelock for the upgrade Safe

[duncancmt]

No description provided.

[ElliotFriedman]

This design works and is well put together. The main issue I see is a limitation on the number of calls you can do. If you try to do more than 1 action at a time, you are going to be signing a lot of different transactions. Say you have a gov proposal that needs to:

1. sweep tokens from a holding address to the multisig
2. approve the tokens for trade
3. call swap, or initiate a TWAP order

All three of these calls are going to need separate multisig transactions and separate signatures from signers, which means you'll have to manage the nonces to make sure the signed tx's are all in the right order. A much better solution would allow delegate calls from the safe, but only after the timelock passes. If your multisig only needs to do a single call, this solution works, if not, it's going to be a rough experience for your signers.

The other thing I see, the contract doesn't store the calldata or target so you can't easily introspect onchain and see what the tx does, making it easier for malicious tx's to slip through.

[duncancmt]

allow delegate calls from the safe ... it's going to be a rough experience for your signers

As I alluded to in the comments in the contract, allowing DELEGATECALL from the Safe means that it's no longer possible to ensure "reasonable" behavior on the part of the Safe by enforcing postcondition checks. Of course this is a bit defense-in-depth. You could simply rely on simulation and the cancel/lockDown game to prevent transactions from unexpectedly modifying the state of the Safe.

Allowing DELEGATECALL would mean enforcing a rather deep inspection on the calls to be performed to ensure that you don't have nested DELEGATECALLs and that none of the calls modify the Safe's own state in unsafe ways. Perhaps this is a "skill issue" and the cost of complexity in this contract is worth the improvement in the number of signatures required on behalf of the signer.

I agree that this ultimately means that the UX of the Safe is significantly degraded. Safe{Wallet} 1.5.0 makes this substantially better by allowing Guards to hook Module-originated calls.

the contract doesn't store the calldata or target

This is emitted in the event when the transaction begins the timelock cooldown.

[immunefi-magnus]

🛡️ Immunefi PR Reviews

This pull request is not eligible for a PR Reviews review. Please contact Immunefi support.

Reason: This PR exceeds the maximum size supported by your current plan (250 lines). Please reduce the number of lines or contact your admin for plan options.