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

Wrong state diff checks during nested approval calls #268

Closed
sebastianst opened this issue Jul 23, 2024 · 2 comments
Closed

Wrong state diff checks during nested approval calls #268

sebastianst opened this issue Jul 23, 2024 · 2 comments

Comments

@sebastianst
Copy link
Member

sebastianst commented Jul 23, 2024

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
Copy link
Contributor

mds1 commented Sep 5, 2024

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");
    }
}

@mds1
Copy link
Contributor

mds1 commented Sep 27, 2024

@maurelian I think we can close this as completed by #319, do you agree?

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

3 participants