ethereum-optimism / superchain-ops

35 stars 31 forks source link

Wrong state diff checks during nested approval calls #268

Open sebastianst opened 1 month ago

sebastianst commented 1 month ago

Nested approval calls seem to run the same state diff checks as the actual task execution. This lead to failing checks during the DGF.setImplementation task executions for Fjord.

It was locally mitigated by removing the task-specific checks and adding code-less address exceptions to the validation script (letting getCodeExceptions() return the SC.getOwners() array). The exceptions were necessary because the SC multisig records all member addresses in a timestamp mapping, and members are mostly EOAs, thus failing the generic code exception checks, that values of state changes that are addresses must contain code.

mds1 commented 2 days ago

My suggestion is to wrap the full contents of the postCheck method in an if (msg.sig != this.approveJson.selector) block. We don't need to run any checks when doing approve calls, so this ensures they only run for simulations and signing.

Example:

/// @notice Checks the correctness of the deployment
function _postCheck(Vm.AccountAccess[] memory accesses, SimulationPayload memory /* simPayload */ )
    internal
    view
    override
{
    if (msg.sig != this.approveJson.selector) {
        console.log("Running post-deploy assertions");

        checkStateDiff(accesses);
        checkSemvers();
        // etc/

        console.log("All assertions passed!");
    } else {
        console.log("Skipping assertions because this is a just approve call");
    }
}