Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/StreamWeightActor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {FVMRewards} from "./lib/FVMRewards.sol";
import {PendingOp, Share, WeightRecord, WeightRecordUpdate} from "./lib/FVMRewardTypes.sol";
import {OwnersLibrary} from "./lib/Owners.sol";
import {UnanimousGovernance} from "./lib/UnanimousGovernance.sol";
import {IsASafe} from "./lib/IsASafe.sol";
import {IsAMultisig} from "./lib/IsAMultisig.sol";

uint64 constant SERVICE_ID = 2;

Expand All @@ -20,20 +20,20 @@ int256 constant STEP = 5e16; // 5%
/// @dev Writes require unanimous owner approval, except `cancelPending`/`cancelPendingWeight`
/// (any single owner, immediate) and `quarterlyGateCheck` (fully permissionless).
contract StreamWeightActor is UnanimousGovernance {
using IsASafe for address;
using IsAMultisig for address;
using OwnersLibrary for address;

IServiceRewardsActor immutable SRA;
Epoch immutable QUARTER;
Epoch immutable HOLD;

/// @notice Deploys the actor with its two initial owners, bound to a Service Rewards Actor.
/// @param owner1 First owner; must be a Safe.
/// @param owner2 Second owner; must be a Safe.
/// @param owner1 First owner; must be a multisig with more than one signer.
/// @param owner2 Second owner; must be a multisig with more than one signer.
/// @param sra Service Rewards Actor supplying QUARTER/HOLD and gating `quarterlyGateCheck`.
constructor(address owner1, address owner2, IServiceRewardsActor sra) {
owner1.isProbablyASafe();
owner2.isProbablyASafe();
owner1.requireMultisig();
owner2.requireMultisig();

owner1.addOwner();
owner2.addOwner();
Expand Down Expand Up @@ -118,9 +118,9 @@ contract StreamWeightActor is UnanimousGovernance {

/// @notice Replaces one of the two owners.
/// @param prevOwner Owner being removed.
/// @param newOwner Owner being added; must be a Safe.
/// @param newOwner Owner being added; must be a multisig with more than one signer.
function replaceOwner(address prevOwner, address newOwner) external unanimousNoHold(keccak256(msg.data)) {
newOwner.isProbablyASafe();
newOwner.requireMultisig();
prevOwner.removeOwner();
newOwner.addOwner();
}
Expand Down
25 changes: 25 additions & 0 deletions src/lib/IsAMultisig.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// SPDX-License-Identifier: Apache-2.0 OR MIT
pragma solidity ^0.8.36;

/// @notice The only thing we require of a multisig: it can list its signers.
/// @dev Safe{Wallet} and the other common multisig implementations all expose this.
interface IMultisig {
function getOwners() external view returns (address[] memory);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Safe implement this method? Can we assume any multisig would implement it? Research others and see what the most-common interface is.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safe definitely does: getOwners() is a public view function on OwnerManager, which Safe inherits.
Just tested with foundry just now to double check but this same 'trick' worked with the Root Key Holders msig-of-msigs which mixed raw and Safe msigs.

But as for others I don't know - what others do we need to support?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we still only support safes.

Here's a list https://www.alchemy.com/dapps/list-of/multisig-wallets-on-ethereum

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool, so we're OK then I think.

If we were still worried and want to be as broad as possible then we could change it to only check address.code.length > 0 like you suggested in the other thread. This is a governance opinion at this point I think.

}

library IsAMultisig {
error NotAMultisig(address account);
error TooFewSigners(address account, uint256 signers);

/// @notice Requires that `account` is a contract behaving like a multisig with >1 signer.
/// @dev FIP-0118 mandates no specific multisig implementation, so this deliberately checks
/// behaviour, not identity: any contract answering `getOwners()` with two or more signers
/// qualifies, and EOAs (no code, so no return data) do not.
function requireMultisig(address account) internal view {
(bool ok, bytes memory ret) = account.staticcall(abi.encodeCall(IMultisig.getOwners, ()));
// 64 bytes is the shortest well-formed `address[]` encoding: head offset + length
require(ok && ret.length >= 64, NotAMultisig(account));
address[] memory signers = abi.decode(ret, (address[]));
require(signers.length > 1, TooFewSigners(account, signers.length));
}
}
18 changes: 0 additions & 18 deletions src/lib/IsASafe.sol

This file was deleted.

49 changes: 34 additions & 15 deletions test/StreamWeightActor.t.sol
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// SPDX-License-Identifier: Apache-2.0 OR MIT
pragma solidity ^0.8.36;

import {SafeProxy} from "@safe/proxies/SafeProxy.sol";

import {USR_FORBIDDEN, USR_ILLEGAL_ARGUMENT, USR_NOT_FOUND} from "fvm-solidity/FVMErrors.sol";

import {MockMultisig} from "./mocks/MockMultisig.sol";
import {MockRewardTest} from "./mocks/MockRewardTest.sol";
import {WAD} from "./mocks/FVMRewardActor.sol";
import {StreamWeightActor} from "../src/StreamWeightActor.sol";
import {IServiceRewardsActor} from "../src/interfaces/IServiceRewardsActor.sol";
import {PendingOp, Share, WeightRecord, WeightRecordUpdate} from "../src/lib/FVMRewardTypes.sol";
import {Epoch} from "../src/lib/Epoch.sol";
import {FVMRewards} from "../src/lib/FVMRewards.sol";
import {IsAMultisig} from "../src/lib/IsAMultisig.sol";
import {SWA_TIMELOCK} from "../src/lib/FVMRewardMethod.sol";

contract StreamWeightActorTest is MockRewardTest {
Expand All @@ -27,8 +27,8 @@ contract StreamWeightActorTest is MockRewardTest {

function setUp() public override {
super.setUp();
owner1 = _makeSafeOwner("owner1");
owner2 = _makeSafeOwner("owner2");
owner1 = _makeMultisigOwner("owner1");
owner2 = _makeMultisigOwner("owner2");

address sra = makeAddr("sra");
vm.mockCall(
Expand All @@ -40,16 +40,9 @@ contract StreamWeightActorTest is MockRewardTest {
rewardActor().mockSwa(address(actor));
}

function _makeSafeOwner(string memory label) internal returns (address proxyAddr) {
address masterCopy = makeAddr(string.concat(label, "-mastercopy"));
vm.etch(masterCopy, new bytes(8001));

SafeProxy real = new SafeProxy(masterCopy);
bytes memory code = address(real).code;

proxyAddr = makeAddr(label);
vm.etch(proxyAddr, code);
vm.store(proxyAddr, bytes32(0), bytes32(uint256(uint160(masterCopy))));
function _makeMultisigOwner(string memory label) internal returns (address multisig) {
multisig = address(new MockMultisig(2));
vm.label(multisig, label);
}

// -------------------------------------------------------------------------
Expand Down Expand Up @@ -285,7 +278,7 @@ contract StreamWeightActorTest is MockRewardTest {
// -------------------------------------------------------------------------

function test_ReplaceOwner_Success_SwapsApprovalRights() public {
address newOwner = _makeSafeOwner("newOwner");
address newOwner = _makeMultisigOwner("newOwner");

vm.prank(owner1);
actor.replaceOwner(owner2, newOwner);
Expand All @@ -301,4 +294,30 @@ contract StreamWeightActorTest is MockRewardTest {
vm.prank(newOwner);
actor.cancelPendingWeight(PendingOp.SET_WEIGHT);
}

// -------------------------------------------------------------------------
// Owner must be a multisig
// -------------------------------------------------------------------------

function test_Constructor_EoaOwner_RevertsNotAMultisig() public {
address eoa = makeAddr("eoa");
vm.expectRevert(abi.encodeWithSelector(IsAMultisig.NotAMultisig.selector, eoa));
new StreamWeightActor(owner1, eoa, IServiceRewardsActor(makeAddr("sra")));
}

function test_Constructor_SingleSignerOwner_RevertsTooFewSigners() public {
address solo = address(new MockMultisig(1));
vm.expectRevert(abi.encodeWithSelector(IsAMultisig.TooFewSigners.selector, solo, uint256(1)));
new StreamWeightActor(owner1, solo, IServiceRewardsActor(makeAddr("sra")));
}

function test_ReplaceOwner_EoaOwner_RevertsNotAMultisig() public {
address eoa = makeAddr("eoa");

vm.prank(owner1);
actor.replaceOwner(owner2, eoa);
vm.prank(owner2);
vm.expectRevert(abi.encodeWithSelector(IsAMultisig.NotAMultisig.selector, eoa));
actor.replaceOwner(owner2, eoa);
}
}
17 changes: 17 additions & 0 deletions test/mocks/MockMultisig.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: Apache-2.0 OR MIT
pragma solidity ^0.8.36;

/// @notice Minimal stand-in for any multisig: it just lists its signers.
contract MockMultisig {
address[] internal signers;

constructor(uint256 count) {
for (uint256 i = 0; i < count; i++) {
signers.push(address(uint160(i + 1)));
}
}

function getOwners() external view returns (address[] memory) {
return signers;
}
}