howlbot-integration[bot] commented 3 weeks ago

Lines of code

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

Vulnerability details

Proof of Concept

From the readme, the contracts are to be deployed on Ethreum, Base, Arbitrum and Optimism. But in SystemDeploy, we can see that the chainIds set are Ethereum, Base, Base-sepolia, and Op-sepolia's instead. Arbitrum's and Op-mainnet's chainId is missing.

We know this because according to the Optimism docs, 11155420 is OP-sepolia's chainId. OP-mainnet's chainId is 10, as mentioned here. Arbitrum's chainIds are mentioned here, 42161 for Arbitrum_One, 42170 for Arbitrum_Nova, both of which are missing from the code's implementation as seen below.

    constructor() {
        uint256[] memory chainIds = new uint256[](4);
        chainIds[0] = 1;
        chainIds[1] = 8453;
        chainIds[2] = 84532;
>>      chainIds[3] = 11155420; //@note no Arbitrum chainId
        addresses = new Addresses("./addresses", chainIds);
    }

As a result, deployment on Arbitrum and Optimism will be impossible

Recommended Mitigation Steps

Recommend setting Arbitrum's chainId and/or changing the OP chainId to 10.

    constructor() {
        uint256[] memory chainIds = new uint256[](5);
        chainIds[0] = 1;
        chainIds[1] = 8453;
        chainIds[2] = 84532;
-       chainIds[3] = 11155420;
+       chainIds[3] = 10;
+       chainIds[4] = 42161; // Or 42170 if NOVA is needed
        addresses = new Addresses("./addresses", chainIds);
    }

Assessed type

Context

ElliotFriedman commented 3 weeks ago

duplicate of an informational, definitely not a medium risk.

the highest rating this should receive is an informational, and even then we already knew the deploy script only supported base and mainnet.

GalloDaSballo commented 3 weeks ago

I agree with the Sponsor that the deployment not including arbitrum cannot be considered a medium severity issue

c4-judge commented 3 weeks ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 3 weeks ago

1 L

c4-judge commented 2 weeks ago

GalloDaSballo marked the issue as grade-b

ZanyBonzy commented 2 weeks ago

Hi @c4-judge, If I may, The main motivation for submitting this is first due to the fact that the depolyment script was set as one of the in-scope contracts to audit. In cases like this, issues in the script had always been validated. For this case, the assumption made during the audit process is that its content have been sufficiently checked by the devs and is believed to stand the audit process. That is why, even simpler issues like common dev oversights are also validated.

For setting a medium risk, according to the judging docs, there are no assets at direct risk, just that the contracts on arbitrum and optimism will not be available to due to the missing chainIds. This breaks deployment functionality to those chains and I believe need to be pointed out(as it wasn't in the known issues). Also, based on some historical decisions, e.g this issue in moonwell contest, in which the deployment script was also in scope, was also validated as med risk and I hope this can provide more context for consistency.

I'd appreciate if your position on this can be reconsidered, based on the provided appeal. Thanks.

DemoreXTess commented 2 weeks ago

52 is dup

GalloDaSballo commented 1 week ago

I appreciate the diligence in pushing this back to the surface as the script was made in scope

I believe that as Judge I have to decide what the finding quality and impacts are I believe the finding must be acknowledged as valid due to it being tied to the inscope code However, I believe it would be incorrect to assert that the lack of the setting will have any specific impact, meaning that in my opinion, QA is the most appropriate severity

code-423n4 / 2024-10-kleidi-findings