Missing Validation to Ensure Hot Signers are Safe Owners

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L52
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L264-L309
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L759-L769

Vulnerability details

Description

In the Timelock contract, there is no check to ensure that the hotSigners are the same as the owners of the safe.

According to the comment on #L:52, "only safe owners can execute whitelisted calldatas," but the constructor and the grantRole() functions do not verify that hot signers are also safe owners.

The separation of the owners array for the safe contract and the hotSigners array for the timelock contract introduces the risk that non-safe owners could be granted the HOT_SIGNER_ROLE and have ablity to execute whitelisted calldatas.

This weakens the intended security model of the system by allowing external or unauthorized addresses to gain the ability to execute privileged actions.

Impact

Non-safe owners may be granted the HOT_SIGNER_ROLE, enabling them to execute whitelisted calldatas without being authorized.

Proof of Concepts

• In Timelock constructor, hot signers are granted HOT_SIGNER_ROLE, without any checks to ensure they are also safe owners as it's stated in #L:52.

• the safe owners array and hotSigners array are separate, and nothing ensures in TimelockFactory that owners[i] == hotsigner[i] when Deploying Timelock.

• The grantRole() function does not validate the account against the list of safe owners to ensure only safe owners can have HOT_SIGNER_ROLE, this allows an arbitrary address to be assigned HOT_SIGNER_ROLE, even if they may not be one of the safe owners.

Recommended mitigation

• Add a check in the Timelock constructor to ensure that the account we are assigning HOT_SIGNER_ROLE to, is one of the safe owners.

• remove hotSigners array from DeploymentParams struct as member and simply pass safe owners array on deployment of Timelock, to assign HOT_SIGNER_ROLE to each safe owner.

• add a check in Timelock::grantRole() function to ensure account is one of the current owners of safe.

Assessed type

Other

[ElliotFriedman]

This is an outdated comment, please refer to the source code and docs for the source of truth.

Disagree with severity, this should be an informational at most.

[GalloDaSballo]

I generally agree with the idea of setting default Hot Signers as Safe Owners, because of an issue I had flagged tied to how a compromised Hot Signer would attack every other chains deployment

That said, this finding pertains to a comment, I think because of it it is valid, but hot signers are not meant to be safe owners as that defeats the purpose, for this reason I think this finding is a good QA finding

[GalloDaSballo]

I have flagged the issue here, hence marking OOS:
https://github.com/GalloDaSballo/kleidi-notes?tab=readme-ov-file#q-24-original-hot-signer-that-becomes-compromised-or-malicious-can-take-over-all-undeployed-systems

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope