MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: low

No Two-Step Ownership Transfer - Risk of Permanent Access Loss

Author Revealed upon completion

Description

  • The contract inherits from OpenZeppelin's Ownable which implements single-step ownership transfer via transferOwnership().

  • If the owner accidentally transfers ownership to an incorrect address (typo, wrong address, non-existent address), ownership is permanently lost with no recovery mechanism.

  • This is especially critical for a multisig wallet where the owner has exclusive power to manage signers and propose transactions.

contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
// @> Inherits Ownable with single-step transfer
// @> No override to implement two-step transfer
constructor() Ownable(msg.sender) {
// Owner is set without pending mechanism
}
}

Risk

Likelihood:

  • Occurs when owner makes a typo or mistake during ownership transfer

  • Common human error scenario

Impact:

  • Permanent loss of owner privileges if transferred to wrong address

  • No ability to add new signers or revoke compromised signers

  • No ability to propose new transactions (per current implementation)

  • Wallet becomes effectively bricked for administrative purposes

Proof of Concept

function testSingleStepOwnershipTransferRisk() public {
// Owner accidentally transfers to wrong address (typo)
address wrongAddress = address(0xDEAD); // Uncontrolled address
multiSigTimelock.transferOwnership(wrongAddress);
// Ownership is immediately transferred - no confirmation needed
assertEq(multiSigTimelock.owner(), wrongAddress);
// Original owner can no longer:
// - Add new signers
vm.expectRevert();
multiSigTimelock.grantSigningRole(makeAddr("newSigner"));
// - Revoke compromised signers
vm.expectRevert();
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
// - Propose transactions
vm.expectRevert();
multiSigTimelock.proposeTransaction(address(0x123), 1 ether, "");
// Wallet is permanently crippled for admin functions
}

Recommended Mitigation

Use OpenZeppelin's Ownable2Step instead of Ownable:

- import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+ import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
​
- contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
+ contract MultiSigTimelock is Ownable2Step, AccessControl, ReentrancyGuard {
constructor() Ownable(msg.sender) {
// Same constructor, but now uses 2-step transfer
}
}

With Ownable2Step:

  1. Current owner calls transferOwnership(newOwner)

  2. New owner must call acceptOwnership() to complete transfer

  3. If wrong address was specified, transfer never completes

Could be seen as best practice/design choice

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!