From f9dca504248a84b03aa4a1b82d0e83aa73ff7430 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 26 Sep 2022 17:40:48 -0600 Subject: [PATCH 01/11] QSP-2 Baal Inherits From Non-Upgradeable Contracts: fixing non upgradeable dependencies on oz --- .vscode/settings.json | 6 ++++++ contracts/Baal.sol | 23 +++++++++++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..9936f68 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,6 @@ +{ + "solidity.defaultCompiler": "localNodeModule", + "solidity.compileUsingRemoteVersion": "v0.8.7+commit.e28d00a7", + "solidity.enableLocalNodeCompiler": false + +} diff --git a/contracts/Baal.sol b/contracts/Baal.sol index 5f19a5b..ff58ceb 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -15,14 +15,14 @@ import "@gnosis.pm/zodiac/contracts/core/Module.sol"; import "@gnosis.pm/safe-contracts/contracts/common/Enum.sol"; import "@opengsn/contracts/src/BaseRelayRecipient.sol"; import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol"; -import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; +import "@openzeppelin/contracts-upgradeable/utils/cryptography/draft-EIP712Upgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; import "./interfaces/IBaalToken.sol"; /// @title Baal ';_;'. /// @notice Flexible guild contract inspired by Moloch DAO framework. -contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { +contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRelayRecipient { using ECDSA for bytes32; // ERC20 SHARES + LOOT @@ -63,7 +63,7 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { // MISCELLANEOUS PARAMS uint32 public latestSponsoredProposalId; /* the id of the last proposal to be sponsored */ address public multisendLibrary; /*address of multisend library*/ - string public override versionRecipient = "2.2.5"; /* version recipient for OpenGSN */ + string public override versionRecipient; /* version recipient for OpenGSN */ // SIGNATURE HELPERS bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint32 proposalId,bool support)"); @@ -218,8 +218,6 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { ); } - constructor() EIP712("Vote", "4") initializer {} /*Configure template to be unusable*/ - /// @notice Summon Baal with voting configuration & initial array of `members` accounts with `shares` & `loot` weights. /// @param _initializationParams Encoded setup information. function setUp(bytes memory _initializationParams) @@ -240,7 +238,16 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { (address, address, address, address, address, bytes) ); + require(_lootToken != address(0), '!lootToken'); + require(_sharesToken != address(0), '!sharesToken'); + require(_multisendLibrary != address(0), '!multisendLibrary'); + require(_avatar != address(0), '!avatar'); + // no need to check _forwarder address exists, the default is address(0) for no forwarder + + versionRecipient = "2.2.5"; __Ownable_init(); + __ReentrancyGuard_init(); + __EIP712_init("Vote", "4"); transferOwnership(_avatar); // Set the Gnosis safe address @@ -707,7 +714,7 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { // **************** // SHAMAN FUNCTIONS // **************** - /// @notice Baal-or-admin-only function to set admin config (pause/unpause shares/loot) and call function on token + /// @notice Baal-or-admin-only function to set admin config (pause/unpause shares/loot) and call function on token /// @param pauseShares Turn share transfers on or off /// @param pauseLoot Turn loot transfers on or off function setAdminConfig(bool pauseShares, bool pauseLoot) @@ -848,7 +855,7 @@ contract Baal is Module, EIP712, ReentrancyGuard, BaseRelayRecipient { function setTrustedForwarder(address _trustedForwarderAddress) external baalOrGovernorOnly - { + { _setTrustedForwarder(_trustedForwarderAddress); } From 6bd908d07fa8c810554f8bcea2abb27207c5c73c Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 26 Sep 2022 17:51:47 -0600 Subject: [PATCH 02/11] QSP-3 Integer Overflow / Underflow - remove unchecked on proposalCount logic --- contracts/Baal.sol | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index ff58ceb..cf8f6ea 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -327,27 +327,25 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela bytes32 proposalDataHash = hashOperation(proposalData); /*Store only hash of proposal data*/ - unchecked { - proposalCount++; /*increment proposal counter*/ - proposals[proposalCount] = Proposal( /*push params into proposal struct - start voting period timer if member submission*/ - proposalCount, - selfSponsor ? latestSponsoredProposalId : 0, /* prevProposalId */ - selfSponsor ? uint32(block.timestamp) : 0, /* votingStarts */ - selfSponsor ? uint32(block.timestamp) + votingPeriod : 0, /* votingEnds */ - selfSponsor - ? uint32(block.timestamp) + votingPeriod + gracePeriod - : 0, /* graceEnds */ - expiration, - baalGas, - 0, /* yes votes */ - 0, /* no votes */ - 0, /* highestMaxSharesAndLootAtYesVote */ - [false, false, false, false], /* [cancelled, processed, passed, actionFailed] */ - selfSponsor ? _msgSender() : address(0), - proposalDataHash, - details - ); - } + proposalCount++; /*increment proposal counter*/ + proposals[proposalCount] = Proposal( /*push params into proposal struct - start voting period timer if member submission*/ + proposalCount, + selfSponsor ? latestSponsoredProposalId : 0, /* prevProposalId */ + selfSponsor ? uint32(block.timestamp) : 0, /* votingStarts */ + selfSponsor ? uint32(block.timestamp) + votingPeriod : 0, /* votingEnds */ + selfSponsor + ? uint32(block.timestamp) + votingPeriod + gracePeriod + : 0, /* graceEnds */ + expiration, + baalGas, + 0, /* yes votes */ + 0, /* no votes */ + 0, /* highestMaxSharesAndLootAtYesVote */ + [false, false, false, false], /* [cancelled, processed, passed, actionFailed] */ + selfSponsor ? _msgSender() : address(0), + proposalDataHash, + details + ); if (selfSponsor) { latestSponsoredProposalId = proposalCount; From e2319d298270b26e36de38b11a80772f5ffcf203 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 26 Sep 2022 18:11:49 -0600 Subject: [PATCH 03/11] QSP-4 Missing Input Validation - reuqiring checks on missed validation --- contracts/Baal.sol | 14 ++++++++++++-- contracts/BaalSummoner.sol | 9 ++++++--- contracts/LootERC20.sol | 3 +++ contracts/SharesERC20.sol | 3 +++ contracts/utils/BaalVotes.sol | 1 + 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index cf8f6ea..3a379db 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -275,7 +275,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela _initializationMultisendData, Enum.Operation.DelegateCall ), - "call failure" + "call failure setup" ); emit SetupComplete( @@ -580,7 +580,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela bytes calldata _data ) external baalOnly { (bool success, ) = _to.call{value: _value}(_data); - require(success, "call failure"); + require(success, "call failure execute"); } // **************** @@ -832,12 +832,22 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela _governanceConfig, (uint32, uint32, uint256, uint256, uint256, uint256) ); + require(quorum >= 0 && minRetention <= 100, 'bad quorum'); + require(minRetention >= 0 && minRetention <= 100, 'bad minRetention'); + + // on initialization of governance config, there is no shares token + // skip this check on initialization of governance config. + if (sponsorThreshold > 0 && address(sharesToken) != address(0)) { + require(sponsor <= totalShares(), 'sponsor > sharesSupply'); + } + if (voting != 0) votingPeriod = voting; /*if positive, reset min. voting periods to first `value`*/ if (grace != 0) gracePeriod = grace; /*if positive, reset grace period to second `value`*/ proposalOffering = newOffering; /*set new proposal offering amount */ quorumPercent = quorum; sponsorThreshold = sponsor; minRetentionPercent = minRetention; + emit GovernanceConfigSet( voting, grace, diff --git a/contracts/BaalSummoner.sol b/contracts/BaalSummoner.sol index 3cd42a8..50f96eb 100644 --- a/contracts/BaalSummoner.sol +++ b/contracts/BaalSummoner.sol @@ -57,13 +57,16 @@ contract BaalSummoner is ModuleProxyFactory { require(_lootSingleton != address(0), "!lootSingleton"); require(_sharesSingleton != address(0), "!sharesSingleton"); require(_gnosisSingleton != address(0), "!gnosisSingleton"); + require(_gnosisFallbackLibrary != address(0), '!gnosisFallbackLibrary'); + require(_gnosisMultisendLibrary != address(0), '!gnosisMultisendLibrary'); + require(_gnosisSafeProxyFactory != address(0), '!gnosisSafeProxyFactory'); + require(_moduleProxyFactory != address(0), '!moduleProxyFactory'); + template = _template; gnosisSingleton = _gnosisSingleton; gnosisFallbackLibrary = _gnosisFallbackLibrary; gnosisMultisendLibrary = _gnosisMultisendLibrary; - gnosisSafeProxyFactory = GnosisSafeProxyFactory( - _gnosisSafeProxyFactory - ); + gnosisSafeProxyFactory = GnosisSafeProxyFactory(_gnosisSafeProxyFactory); moduleProxyFactory = ModuleProxyFactory(_moduleProxyFactory); lootSingleton = _lootSingleton; sharesSingleton = _sharesSingleton; diff --git a/contracts/LootERC20.sol b/contracts/LootERC20.sol index e8ef176..d5654b0 100644 --- a/contracts/LootERC20.sol +++ b/contracts/LootERC20.sol @@ -28,6 +28,9 @@ contract Loot is external initializer { + require(bytes(name_).length != 0, "Lname empty"); + require(bytes(symbol_).length != 0, "Lsymbol empty"); + __ERC20_init(name_, symbol_); __ERC20Permit_init(name_); __Pausable_init(); diff --git a/contracts/SharesERC20.sol b/contracts/SharesERC20.sol index 9440d01..0988d36 100644 --- a/contracts/SharesERC20.sol +++ b/contracts/SharesERC20.sol @@ -28,6 +28,9 @@ contract Shares is BaalVotes, OwnableUpgradeable, PausableUpgradeable, UUPSUpgra external initializer { + require(bytes(name_).length != 0, "Sname empty"); + require(bytes(symbol_).length != 0, "Ssymbol empty"); + __ERC20_init(name_, symbol_); __ERC20Permit_init(name_); __Pausable_init(); diff --git a/contracts/utils/BaalVotes.sol b/contracts/utils/BaalVotes.sol index 1a91169..a6baff5 100644 --- a/contracts/utils/BaalVotes.sol +++ b/contracts/utils/BaalVotes.sol @@ -94,6 +94,7 @@ abstract contract BaalVotes is ERC20PermitUpgradeable { r, s ); + require(signer != address(0), "ERC20Votes: invalid signer (0x0)"); require(nonce == _useNonce(signer), "ERC20Votes: invalid nonce"); _delegate(signer, delegatee); } From 62396f82cc5064a6f91e56062bcde93eb798f12b Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Wed, 28 Sep 2022 14:51:06 -0600 Subject: [PATCH 04/11] QSP-8 Application Monitoring Can Be Improved by Emitting More Events --- contracts/Baal.sol | 11 +++++++- contracts/tools/TributeMinion.sol | 46 ++++++++++++++++++++++++++----- test/BaalSafe.test.ts | 16 +++++------ 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index 3a379db..24a6105 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -195,6 +195,9 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela ); /*emits when gov config changes*/ event SharesPaused(bool paused); /*emits when shares are paused or unpaused*/ event LootPaused(bool paused); /*emits when loot is paused or unpaused*/ + event LockAdmin(bool adminLock); /*emits when admin is locked*/ + event LockManager(bool managerLock); /*emits when admin is locked*/ + event LockGovernor(bool governorLock); /*emits when admin is locked*/ function encodeMultisend(bytes[] memory _calls, address _target) external @@ -697,16 +700,22 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela /// @notice Lock admin so setShamans cannot be called with admin changes function lockAdmin() external baalOnly { adminLock = true; + + emit LockAdmin(adminLock); } /// @notice Lock manager so setShamans cannot be called with manager changes function lockManager() external baalOnly { managerLock = true; + + emit LockManager(managerLock); } /// @notice Lock governor so setShamans cannot be called with governor changes function lockGovernor() external baalOnly { governorLock = true; + + emit LockGovernor(governorLock); } // **************** @@ -834,7 +843,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela ); require(quorum >= 0 && minRetention <= 100, 'bad quorum'); require(minRetention >= 0 && minRetention <= 100, 'bad minRetention'); - + // on initialization of governance config, there is no shares token // skip this check on initialization of governance config. if (sponsorThreshold > 0 && address(sharesToken) != address(0)) { diff --git a/contracts/tools/TributeMinion.sol b/contracts/tools/TributeMinion.sol index 5c521cb..c96fd4f 100644 --- a/contracts/tools/TributeMinion.sol +++ b/contracts/tools/TributeMinion.sol @@ -3,11 +3,21 @@ pragma solidity 0.8.7; import "../Baal.sol"; interface IERC20 { - function transferFrom(address from, address to, uint256 value) external returns (bool); + function transferFrom( + address from, + address to, + uint256 value + ) external returns (bool); } contract TributeMinion { - event TributeProposal(address indexed baal, address token, uint256 amount, address recipient, uint256 proposalId); + event TributeProposal( + address indexed baal, + address token, + uint256 amount, + address recipient, + uint256 proposalId + ); struct Escrow { address token; address applicant; @@ -17,6 +27,14 @@ contract TributeMinion { } mapping(address => mapping(uint256 => Escrow)) public escrows; + event EscrowReleased( + address indexed baal, + uint32 proposalId, + address applicant, + address safe, + uint256 amount + ); + function encodeTributeProposal( address baal, uint256 shares, @@ -102,6 +120,7 @@ contract TributeMinion { string memory details ) external payable { uint32 proposalId = baal.proposalCount() + 1; + bytes memory encodedProposal = encodeTributeProposal( address(baal), shares, @@ -110,6 +129,7 @@ contract TributeMinion { proposalId, address(this) ); + escrows[address(baal)][proposalId] = Escrow( token, msg.sender, @@ -117,24 +137,36 @@ contract TributeMinion { false, baal.target() ); + baal.submitProposal{value:msg.value}(encodedProposal, expiration, baalgas, details); - emit TributeProposal(address(baal), token, amount, msg.sender, proposalId); + + emit TributeProposal( + address(baal), + token, + amount, + msg.sender, + proposalId + ); } function releaseEscrow(address _baal, uint32 _proposalId) external { - // console.log("releasing"); Baal baal = Baal(_baal); Escrow storage escrow = escrows[address(baal)][_proposalId]; require(!escrow.released, "Already released"); - // console.log("releasing1b"); bool[4] memory status = baal.getProposalStatus(_proposalId); - // console.log("releasing1c"); require(status[2], "Not passed"); escrow.released = true; IERC20 token = IERC20(escrow.token); - // console.log("releasing2"); + + emit EscrowReleased( + _baal, + _proposalId, + escrow.applicant, + escrow.safe, + escrow.amount + ); require( token.transferFrom(escrow.applicant, escrow.safe, escrow.amount), diff --git a/test/BaalSafe.test.ts b/test/BaalSafe.test.ts index 97b861f..4a2376a 100644 --- a/test/BaalSafe.test.ts +++ b/test/BaalSafe.test.ts @@ -157,7 +157,7 @@ const getBaalParams = async function ( shamans: [string[], number[]], shares: [string[], number[]], loots: [string[], number[]], - trustedForwarder: string, + trustedForwarder: string, ) { const governanceConfig = abiCoder.encode( ["uint32", "uint32", "uint256", "uint256", "uint256", "uint256"], @@ -197,7 +197,7 @@ const getBaalParams = async function ( const posterFromBaal = await baal.interface.encodeFunctionData( "executeAsBaal", [poster.address, 0, postMetaData] - ); + ); const initalizationActions = [ setAdminConfig, @@ -644,11 +644,11 @@ describe("Baal contract", function () { it("can eject and upgrade token with eoa", async function () { // upgrade token contracts to remove baal deps // call from safe - // remove baal module + // remove baal module // owner should be baal expect(await sharesToken.owner()).to.equal(baal.address); - + const transferOwnershipAction = await sharesToken.interface.encodeFunctionData( "transferOwnership", [summoner.address] @@ -665,7 +665,7 @@ describe("Baal contract", function () { .withArgs(1, true, false); expect(await sharesToken.owner()).to.equal(summoner.address); - + let BaalLessSharesFactory: ContractFactory; BaalLessSharesFactory = await ethers.getContractFactory("BaalLessShares"); @@ -674,9 +674,9 @@ describe("Baal contract", function () { const sharesTokenAsOwnerEoa = await sharesToken.connect(summoner); expect(await baalLessSharesSingleton.version()).to.equal(0); console.log('upgrade'); - + await sharesTokenAsOwnerEoa.upgradeToAndCall( - baalLessSharesSingleton.address, + baalLessSharesSingleton.address, baalLessSharesSingleton.interface.encodeFunctionData("setUp", [ 2 ])) @@ -692,7 +692,7 @@ describe("Baal contract", function () { expect(await sharesToken.balanceOf(summoner.address)).to.equal(200); - + }); }); describe("shaman actions - permission level 7 (full)", function () { From 9b5c4b0198df61473fab1a9f92551bbb05402ef0 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Wed, 28 Sep 2022 16:07:50 -0600 Subject: [PATCH 05/11] QSP-7 Signed Votes Do Not Expire --- contracts/Baal.sol | 11 +++++++---- src/signVote.ts | 3 +++ test/BaalSafe.test.ts | 20 +++++++++++--------- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index 24a6105..2624d9b 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -14,7 +14,6 @@ import "@gnosis.pm/safe-contracts/contracts/GnosisSafe.sol"; import "@gnosis.pm/zodiac/contracts/core/Module.sol"; import "@gnosis.pm/safe-contracts/contracts/common/Enum.sol"; import "@opengsn/contracts/src/BaseRelayRecipient.sol"; -import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import "@openzeppelin/contracts-upgradeable/utils/cryptography/draft-EIP712Upgradeable.sol"; import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; @@ -23,7 +22,7 @@ import "./interfaces/IBaalToken.sol"; /// @title Baal ';_;'. /// @notice Flexible guild contract inspired by Moloch DAO framework. contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRelayRecipient { - using ECDSA for bytes32; + using ECDSAUpgradeable for bytes32; // ERC20 SHARES + LOOT @@ -66,7 +65,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela string public override versionRecipient; /* version recipient for OpenGSN */ // SIGNATURE HELPERS - bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint32 proposalId,bool support)"); + bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint256 expiry,uint32 proposalId,bool support)"); // DATA STRUCTURES struct Proposal { @@ -408,6 +407,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela /// @notice Submit vote with EIP-712 signature - proposal must exist & voting period must not have ended. /// @param voter Address of member who submitted vote. + /// @param expiry Expiration of signature. /// @param id Number of proposal in `proposals` mapping to cast vote on. /// @param approved If 'true', member will cast `yesVotes` onto proposal - if 'false', `noVotes` will be counted. /// @param v v in signature @@ -415,24 +415,27 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela /// @param s s in signature function submitVoteWithSig( address voter, + uint256 expiry, uint32 id, bool approved, uint8 v, bytes32 r, bytes32 s ) external nonReentrant { + require(block.timestamp <= expiry, "ERC20Votes: signature expired"); /*calculate EIP-712 struct hash*/ bytes32 structHash = keccak256( abi.encode( VOTE_TYPEHASH, keccak256(abi.encodePacked(sharesToken.name())), voter, + expiry, id, approved ) ); bytes32 hash = _hashTypedDataV4(structHash); - address signer = ECDSA.recover(hash, v, r, s); + address signer = ECDSAUpgradeable.recover(hash, v, r, s); require(signer == voter, "invalid signature"); require(signer != address(0), "!signer"); diff --git a/src/signVote.ts b/src/signVote.ts index b05ae8e..ff5cd73 100644 --- a/src/signVote.ts +++ b/src/signVote.ts @@ -5,6 +5,7 @@ export default async function signVote( contractAddress: string, signer: SignerWithAddress, name: string, + expiry: number, proposalId: number, support: boolean @@ -20,6 +21,7 @@ export default async function signVote( Vote: [ { name: 'name', type: 'string' }, { name: 'voter', type: 'address' }, + { name: 'expiry', type: 'uint256' }, { name: 'proposalId', type: 'uint32' }, { name: 'support', type: 'bool' }, ], @@ -28,6 +30,7 @@ export default async function signVote( const values = { name, voter: signer.address, + expiry, proposalId, support } diff --git a/test/BaalSafe.test.ts b/test/BaalSafe.test.ts index 4a2376a..a0a9653 100644 --- a/test/BaalSafe.test.ts +++ b/test/BaalSafe.test.ts @@ -25,15 +25,10 @@ import { hashOperation, } from "../src/util"; import { BigNumber, BigNumberish } from "@ethersproject/bignumber"; -import { buildContractCall } from "@gnosis.pm/safe-contracts"; import { ContractFactory, ContractTransaction, utils } from "ethers"; -import { Test } from "mocha"; import signVote from "../src/signVote"; import signDelegation from "../src/signDelegation"; -import signPermit from "../src/signPermit"; -import { string } from "hardhat/internal/core/params/argumentTypes"; import { calculateProxyAddress } from "@gnosis.pm/zodiac"; -import { Address } from "cluster"; use(solidity); @@ -2508,17 +2503,19 @@ describe("Baal contract", function () { }); it("happy case - yes vote", async function () { + const expiry = await blockTime() + 1200; const signature = await signVote( chainId, baal.address, summoner, deploymentConfig.TOKEN_NAME, + expiry, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); - await baal.submitVoteWithSig(summoner.address, 1, true, v, r, s); + await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s); const prop = await baal.proposals(1); const nCheckpoints = await sharesToken.numCheckpoints(summoner.address); const votes = ( @@ -2532,37 +2529,42 @@ describe("Baal contract", function () { expect(prop.yesVotes).to.equal(votes); }); + it("fail case - fails with different voter", async function () { + const expiry = await blockTime() + 1200; const signature = await signVote( chainId, baal.address, summoner, deploymentConfig.TOKEN_NAME, + expiry, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); expect( - baal.submitVoteWithSig(applicant.address, 1, true, v, r, s) + baal.submitVoteWithSig(applicant.address, expiry, 1, true, v, r, s) ).to.be.revertedWith("invalid signature"); }); it("fail case - cant vote twice", async function () { + const expiry = await blockTime() + 1200; const signature = await signVote( chainId, baal.address, summoner, deploymentConfig.TOKEN_NAME, + expiry, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); - await baal.submitVoteWithSig(summoner.address, 1, true, v, r, s); + await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s); expect( - baal.submitVoteWithSig(summoner.address, 1, true, v, r, s) + baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s) ).to.be.revertedWith("voted"); }); }); From 3c1792a94c815e1d4df74ff7ec266118f66ed4b9 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Wed, 28 Sep 2022 16:08:51 -0600 Subject: [PATCH 06/11] QSP-9 setAdminConfig Always Emits Two Events Even if State Is Not Changed. --- contracts/Baal.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index 2624d9b..feae793 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -734,17 +734,19 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela if(pauseShares && !sharesToken.paused()){ sharesToken.pause(); + emit SharesPaused(true); } else if(!pauseShares && sharesToken.paused()){ sharesToken.unpause(); + emit SharesPaused(false); } + if(pauseLoot && !lootToken.paused()){ lootToken.pause(); + emit LootPaused(true); } else if(!pauseLoot && lootToken.paused()){ lootToken.unpause(); + emit LootPaused(false); } - - emit SharesPaused(pauseShares); - emit LootPaused(pauseLoot); } /// @notice Baal-or-manager-only function to mint shares. From 68e53af869af4e5c7084c02eae966da78a1a0f00 Mon Sep 17 00:00:00 2001 From: Dekan Date: Mon, 3 Oct 2022 10:46:53 -0600 Subject: [PATCH 07/11] QSP-1 Checkpoints May Not Be Written Correctly (#77) Co-authored-by: dekanbro --- contracts/utils/BaalVotes.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/utils/BaalVotes.sol b/contracts/utils/BaalVotes.sol index a6baff5..18a5b54 100644 --- a/contracts/utils/BaalVotes.sol +++ b/contracts/utils/BaalVotes.sol @@ -164,10 +164,9 @@ abstract contract BaalVotes is ERC20PermitUpgradeable { unchecked { if ( nCheckpoints != 0 && - getCheckpoint(delegatee, nCheckpoints - 1).fromTimeStamp == - timeStamp + checkpoints[delegatee][nCheckpoints - 1].fromTimeStamp == timeStamp ) { - getCheckpoint(delegatee, nCheckpoints - 1).votes = newVotes; + checkpoints[delegatee][nCheckpoints - 1].votes = newVotes; } else { checkpoints[delegatee][nCheckpoints] = Checkpoint( timeStamp, From c20ee60a305806766200f2f1aedb6ace4acb7ad0 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 3 Oct 2022 10:58:10 -0600 Subject: [PATCH 08/11] QSP-16 Proposals Can Pass without a Valid Sponsor --- contracts/Baal.sol | 3 ++- test/BaalSafe.test.ts | 22 ++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index feae793..dd35600 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -488,7 +488,8 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela { Proposal storage prop = proposals[id]; /*alias `proposal` storage pointers*/ - require(state(id) == ProposalState.Ready, "!ready"); + require(prop.sponsor != address(0), "!sponsor"); /*check proposal has been sponsored*/ + require(state(id) == ProposalState.Ready, "!ready"); /* check proposal is Ready to process */ ProposalState prevProposalState = state(prop.prevProposalId); require( diff --git a/test/BaalSafe.test.ts b/test/BaalSafe.test.ts index a0a9653..b9da9f0 100644 --- a/test/BaalSafe.test.ts +++ b/test/BaalSafe.test.ts @@ -44,7 +44,7 @@ const revertMessages = { submitProposalOffering: "Baal requires an offering", submitVoteTimeEnded: "ended", sponsorProposalExpired: "expired", - sponsorProposalSponsor: "!sponsor", + proposalNotSponsored: "!sponsor", sponsorProposalNotSubmitted: "!submitted", submitVoteNotSponsored: "!sponsored", submitVoteNotVoting: "!voting", @@ -54,6 +54,7 @@ const revertMessages = { submitVoteWithSigVoted: "voted", submitVoteWithSigMember: "!member", processProposalNotReady: "!ready", + proposalNotSponsored: '!sponsor', ragequitUnordered: "!order", // unsetGuildTokensLastToken: 'reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)', sharesTransferPaused: "!transferable", @@ -2300,7 +2301,7 @@ describe("Baal contract", function () { ); await expect(shamanBaal.sponsorProposal(1)).to.be.revertedWith( - revertMessages.sponsorProposalSponsor + revertMessages.proposalNotSponsored ); }); @@ -2725,8 +2726,21 @@ describe("Baal contract", function () { await baal.submitVote(1, yes); const state = await baal.state(2); expect(state).to.equal(STATES.UNBORN); + }); + + it("require fail - no sponser", async function () { + await baal.submitProposal( + proposal.data, + proposal.expiration, + proposal.baalGas, + ethers.utils.id(proposal.details) + ); + await baal.submitVote(1, yes); + const state = await baal.state(2); + expect(state).to.equal(STATES.UNBORN); + proposal.sponsor = null; await expect(baal.processProposal(2, proposal.data)).to.be.revertedWith( - revertMessages.processProposalNotReady + revertMessages.proposalNotSponsored ); }); @@ -2780,7 +2794,7 @@ describe("Baal contract", function () { ethers.utils.id(proposal.details) ); await expect(baal.processProposal(1, proposal.data)).to.be.revertedWith( - revertMessages.processProposalNotReady + revertMessages.proposalNotSponsored ); // fail at submitted await baal.sponsorProposal(1); await expect(baal.processProposal(1, proposal.data)).to.be.revertedWith( From 67dfe26aadc063a88d223bb04b50a4b60d80803c Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 3 Oct 2022 12:10:48 -0600 Subject: [PATCH 09/11] updating version recipient --- contracts/Baal.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index dd35600..a827218 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -246,7 +246,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela require(_avatar != address(0), '!avatar'); // no need to check _forwarder address exists, the default is address(0) for no forwarder - versionRecipient = "2.2.5"; + versionRecipient = "2.2.5+opengsn.payablewithbaal.irelayrecipient"; __Ownable_init(); __ReentrancyGuard_init(); __EIP712_init("Vote", "4"); From 143f9dc54a0195c784d5abff770748c266d7ddea Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 3 Oct 2022 12:11:02 -0600 Subject: [PATCH 10/11] adding nonce to submitVote --- contracts/Baal.sol | 8 +++++++- src/signVote.ts | 3 +++ test/BaalSafe.test.ts | 26 +++++++++++++++++++++----- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/contracts/Baal.sol b/contracts/Baal.sol index a827218..59bb23a 100644 --- a/contracts/Baal.sol +++ b/contracts/Baal.sol @@ -57,6 +57,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela // PROPOSAL TRACKING mapping(address => mapping(uint32 => bool)) public memberVoted; /*maps members to their proposal votes (true = voted) */ + mapping(address => uint256) public votingNonces; /*maps members to their voting nonce*/ mapping(uint256 => Proposal) public proposals; /*maps `proposal id` to struct details*/ // MISCELLANEOUS PARAMS @@ -65,7 +66,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela string public override versionRecipient; /* version recipient for OpenGSN */ // SIGNATURE HELPERS - bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint256 expiry,uint32 proposalId,bool support)"); + bytes32 constant VOTE_TYPEHASH = keccak256("Vote(string name,address voter,uint256 expiry,uint256 nonce,uint32 proposalId,bool support)"); // DATA STRUCTURES struct Proposal { @@ -416,6 +417,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela function submitVoteWithSig( address voter, uint256 expiry, + uint256 nonce, uint32 id, bool approved, uint8 v, @@ -423,6 +425,8 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela bytes32 s ) external nonReentrant { require(block.timestamp <= expiry, "ERC20Votes: signature expired"); + require(nonce == votingNonces[voter], "!nonce"); + /*calculate EIP-712 struct hash*/ bytes32 structHash = keccak256( abi.encode( @@ -430,6 +434,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela keccak256(abi.encodePacked(sharesToken.name())), voter, expiry, + nonce, id, approved ) @@ -439,6 +444,7 @@ contract Baal is Module, EIP712Upgradeable, ReentrancyGuardUpgradeable, BaseRela require(signer == voter, "invalid signature"); require(signer != address(0), "!signer"); + votingNonces[voter] += 1; _submitVote(signer, id, approved); } diff --git a/src/signVote.ts b/src/signVote.ts index ff5cd73..448badc 100644 --- a/src/signVote.ts +++ b/src/signVote.ts @@ -6,6 +6,7 @@ export default async function signVote( signer: SignerWithAddress, name: string, expiry: number, + nonce: number, proposalId: number, support: boolean @@ -22,6 +23,7 @@ export default async function signVote( { name: 'name', type: 'string' }, { name: 'voter', type: 'address' }, { name: 'expiry', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, { name: 'proposalId', type: 'uint32' }, { name: 'support', type: 'bool' }, ], @@ -31,6 +33,7 @@ export default async function signVote( name, voter: signer.address, expiry, + nonce, proposalId, support } diff --git a/test/BaalSafe.test.ts b/test/BaalSafe.test.ts index b9da9f0..a2377e3 100644 --- a/test/BaalSafe.test.ts +++ b/test/BaalSafe.test.ts @@ -54,7 +54,6 @@ const revertMessages = { submitVoteWithSigVoted: "voted", submitVoteWithSigMember: "!member", processProposalNotReady: "!ready", - proposalNotSponsored: '!sponsor', ragequitUnordered: "!order", // unsetGuildTokensLastToken: 'reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)', sharesTransferPaused: "!transferable", @@ -2511,12 +2510,13 @@ describe("Baal contract", function () { summoner, deploymentConfig.TOKEN_NAME, expiry, + 0, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); - await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s); + await baal.submitVoteWithSig(summoner.address, expiry, 0, 1, true, v, r, s); const prop = await baal.proposals(1); const nCheckpoints = await sharesToken.numCheckpoints(summoner.address); const votes = ( @@ -2526,6 +2526,7 @@ describe("Baal contract", function () { summoner.address, prop.votingStarts ); + expect(await baal.votingNonces(summoner.address)).to.equal(1); expect(priorVotes).to.equal(votes); expect(prop.yesVotes).to.equal(votes); }); @@ -2539,14 +2540,16 @@ describe("Baal contract", function () { summoner, deploymentConfig.TOKEN_NAME, expiry, + 0, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); expect( - baal.submitVoteWithSig(applicant.address, expiry, 1, true, v, r, s) + baal.submitVoteWithSig(applicant.address, expiry, 0, 1, true, v, r, s) ).to.be.revertedWith("invalid signature"); + expect(await baal.votingNonces(applicant.address)).to.equal(0); }); it("fail case - cant vote twice", async function () { @@ -2557,16 +2560,29 @@ describe("Baal contract", function () { summoner, deploymentConfig.TOKEN_NAME, expiry, + 0, 1, true ); const {v,r,s} = await ethers.utils.splitSignature(signature); - await baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s); + await baal.submitVoteWithSig(summoner.address, expiry, 0, 1, true, v, r, s); + const signatureTwo = await signVote( + chainId, + baal.address, + summoner, + deploymentConfig.TOKEN_NAME, + expiry, + 1, + 1, + true + ); + const sigTwo = await ethers.utils.splitSignature(signatureTwo); expect( - baal.submitVoteWithSig(summoner.address, expiry, 1, true, v, r, s) + baal.submitVoteWithSig(summoner.address, expiry, 1, 1, true, sigTwo.v, sigTwo.r, sigTwo.s) ).to.be.revertedWith("voted"); + expect(await baal.votingNonces(summoner.address)).to.equal(1); }); }); From cd28a98009b31eb2df5441983e0224ba96d0bf94 Mon Sep 17 00:00:00 2001 From: brianrossetti Date: Mon, 3 Oct 2022 14:46:16 -0600 Subject: [PATCH 11/11] adding docs for running coverage --- README.md | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2657d22..d648d5f 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,38 @@ the dao contracts and the Safe treasury and one to use an existing Safe treasury - ./tasks - hard hat cli tasks - ./tests - test files +---- + +## Coverage + +currently, coverage is turned off for test efficiency purposes. In order to switch coverage on, add `yul` to the hardhat config: + +``` +{ + ... + compilers: [ + { + version: "0.8.7", + settings: { + optimizer: { + enabled: true, + runs: 200, + details: { + yul: true + } + }, + }, + } + ] +} +``` + +then run the coverage command: + +``` +npx hardhat coverage +``` + ---- ## Privileged roles - Shamans - are specific addresses that have more granular control @@ -137,4 +169,4 @@ beta release of the factories. These factories may change until we get to final - baalSingleton: 0xDb3e9Ded9843358fbbe758c4e73cCfEb9061d4Ed - Transaction Hash: 0x703acad6f005fa793e9041acc711a3d1ac4c8b632898c08598552d024687bc06 - Factory Contract Address: **0x3Bd3fDf6db732F8548638Cd35B98d624c77FB351** -- Tribute Minion: 0x9391b6A7c55832a6802484dE054d81496D56545A \ No newline at end of file +- Tribute Minion: 0x9391b6A7c55832a6802484dE054d81496D56545A