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

Deploy script is trying to deploy to wrong chains #52

Open
howlbot-integration bot opened this issue Oct 27, 2024 · 2 comments
Open

Deploy script is trying to deploy to wrong chains #52

howlbot-integration bot opened this issue Oct 27, 2024 · 2 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-22 edited-by-warden grade-b Q-09 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_22_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/deploy/SystemDeploy.s.sol#L23-L26

Vulnerability details

Proof of Concept

In the README file, the protocol states that the system deployment will be deployed to Arbitrum, Ethereum Mainnet, Base, Optimism Mainnet.

Chains the protocol will be deployed on Arbitrum,Ethereum,Optimism,Base

In Kleidi protocol the deploy script is also in the scope in this contest. But the deploy script is using wrong chain id's in the constructor and deployment won't work on Optimism and Arbitrum.

    constructor() {
        uint256[] memory chainIds = new uint256[](4);
        chainIds[0] = 1; // @audit-info Mainnet
        chainIds[1] = 8453; // @audit-info Base
        chainIds[2] = 84532; // @audit-info Base Sepolia
        chainIds[3] = 11155420; // @audit-info Optimism Sepolia
        addresses = new Addresses("./addresses", chainIds);
    }

And the provided deployment command won't work in Optimism and Arbitrum.

/// DO_PRINT=false DO_BUILD=false DO_RUN=false DO_DEPLOY=true DO_VALIDATE=true forge script src/deploy/SystemDeploy.s.sol:SystemDeploy --fork-url base -vvvvv

Because in validation it will revert.

    function validate() public view override {
        if (addresses.isAddressSet("TIMELOCK_FACTORY")) {
            address factory = addresses.getAddress("TIMELOCK_FACTORY");
            assertEq(
                keccak256(factory.code),
                keccak256(type(TimelockFactory).runtimeCode),
                "Incorrect TimelockFactory Bytecode"
            );

            address guard = addresses.getAddress("GUARD");
            assertEq(
                keccak256(guard.code),
                keccak256(type(Guard).runtimeCode),
                "Incorrect Guard Bytecode"
            );

            address recoverySpellFactory =
                addresses.getAddress("RECOVERY_SPELL_FACTORY");
            assertEq(
                keccak256(recoverySpellFactory.code),
                keccak256(type(RecoverySpellFactory).runtimeCode),
                "Incorrect RecoverySpellFactory Bytecode"
            );

            /// cannot check bytecode, following error is thrown when trying:
            ///  `"runtimeCode" is not available for contracts containing
            ///   immutable variables.`
            InstanceDeployer deployer =
                InstanceDeployer(addresses.getAddress("INSTANCE_DEPLOYER"));

            assertEq(
                deployer.safeProxyFactory(),
                addresses.getAddress("SAFE_FACTORY"),
                "incorrect safe proxy factory"
            );
            assertEq(
                deployer.safeProxyLogic(),
                addresses.getAddress("SAFE_LOGIC"),
                "incorrect safe logic contract"
            );
            assertEq(
                deployer.timelockFactory(),
                addresses.getAddress("TIMELOCK_FACTORY"),
                "incorrect timelock factory"
            );
            assertEq(
                deployer.guard(),
                addresses.getAddress("GUARD"),
                "incorrect GUARD"
            );
            assertEq(
                deployer.multicall3(),
                addresses.getAddress("MULTICALL3"),
                "incorrect MULTICALL3"
            );
        }
    }

Recommended Mitigation Steps

Swap the correct chainIds with the wrong chainIds.

Assessed type

Context

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_22_group AI based duplicate group recommendation bug Something isn't working duplicate-22 edited-by-warden sufficient quality report This report is of sufficient quality labels Oct 27, 2024
howlbot-integration bot added a commit that referenced this issue Oct 27, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 30, 2024
@c4-judge
Copy link

GalloDaSballo changed the severity to QA (Quality Assurance)

@c4-judge
Copy link

c4-judge commented Nov 4, 2024

GalloDaSballo marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-22 edited-by-warden grade-b Q-09 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_22_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants