Root + Impact (High)
Description
-
The contract advertises multi-signature security but concentrates critical powers in a single owner address.
-
The owner unilaterally controls transaction proposals and signer membership without requiring any multi-signature approval.
-
This creates a single point of failure where one compromised key enables complete fund drainage, rendering the 3-of-5 signature requirement security theater.
function proposeTransaction(...) external onlyOwner
@>
function grantSigningRole(...) external onlyOwner
@>
function revokeSigningRole(...) external onlyOwner
@>
Risk
Likelihood: (Medium)
-
Owner key compromised via phishing, social engineering, or insider threat
-
Single key compromise vs. requiring 3 separate key compromises
Impact: (High)
-
Compromised owner replaces all signers with attacker-controlled addresses
-
Proposes and executes arbitrary transactions draining all funds
-
Multi-signature requirement provides false security assurance
Proof of Concept
Demonstrates complete contract drainage through single owner key compromise, requiring zero cooperation from legitimate signers.
function test_OwnerSinglePointOfFailure() public grantSigningRoles {
vm.deal(address(multiSigTimelock), 100 ether);
address COLLUDER_1 = makeAddr("colluder_1");
address COLLUDER_2 = makeAddr("colluder_2");
address COLLUDER_3 = makeAddr("colluder_3");
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
multiSigTimelock.revokeSigningRole(SIGNER_FOUR);
multiSigTimelock.revokeSigningRole(SIGNER_FIVE);
multiSigTimelock.grantSigningRole(COLLUDER_1);
multiSigTimelock.grantSigningRole(COLLUDER_2);
multiSigTimelock.grantSigningRole(COLLUDER_3);
uint256 txnId = multiSigTimelock.proposeTransaction(ATTACKER, 100 ether, "");
vm.prank(COLLUDER_1);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(COLLUDER_2);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(COLLUDER_3);
multiSigTimelock.confirmTransaction(txnId);
vm.warp(block.timestamp + 7 days + 1);
vm.prank(COLLUDER_1);
multiSigTimelock.executeTransaction(txnId);
assertEq(ATTACKER.balance, 100 ether);
assertEq(address(multiSigTimelock).balance, 0);
}
Recommended Mitigation
Apply multi-signature requirements to all critical operations, not just execution.
+ // Require multi-sig for signer management
+ struct SignerProposal {
+ address account;
+ bool isAddition; // true = add, false = remove
+ uint256 approvals;
+ mapping(address => bool) hasApproved;
+ }
+ mapping(uint256 => SignerProposal) public signerProposals;
- function grantSigningRole(address _account) external onlyOwner {
+ function proposeSignerChange(address _account, bool _isAddition) external onlyRole(SIGNING_ROLE) {
+ // Creates proposal requiring 3 approvals
+ }
+ function approveSignerChange(uint256 proposalId) external onlyRole(SIGNING_ROLE) {
+ // Increment approvals, execute if threshold met
+ }
- function proposeTransaction(...) external onlyOwner {
+ function proposeTransaction(...) external onlyRole(SIGNING_ROLE) {
+ // Any signer can propose, still needs 3 confirmations to execute
+ }
