From 4d43a5a02fe5fe7734ad9e7b2dcb26d2f3ba1f58 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Fri, 15 Sep 2023 14:10:13 -0400 Subject: [PATCH 01/10] fix: mitigations contributor router --- contracts/crowdfund/ContributionRouter.sol | 29 +++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/contracts/crowdfund/ContributionRouter.sol b/contracts/crowdfund/ContributionRouter.sol index 109b9beb..e6608869 100644 --- a/contracts/crowdfund/ContributionRouter.sol +++ b/contracts/crowdfund/ContributionRouter.sol @@ -18,12 +18,16 @@ contract ContributionRouter { /// @notice The address allowed to claim fees from the contract. address public immutable OWNER; - /// @notice The amount of fees to pay to the DAO per mint. - uint96 public feePerMint; + struct Storage { + uint96 feePerMint; + address caller; + } + + Storage private _storage; constructor(address owner, uint96 initialFeePerMint) { OWNER = owner; - feePerMint = initialFeePerMint; + _storage.feePerMint = initialFeePerMint; } modifier onlyOwner() { @@ -34,9 +38,9 @@ contract ContributionRouter { /// @notice Set the fee per mint. Only the owner can call. /// @param newFeePerMint The new amount to set fee per mint to. function setFeePerMint(uint96 newFeePerMint) external onlyOwner { - emit FeePerMintUpdated(feePerMint, newFeePerMint); + emit FeePerMintUpdated(_storage.feePerMint, newFeePerMint); - feePerMint = newFeePerMint; + _storage.feePerMint = newFeePerMint; } /// @notice Claim fees from the contract. Only the owner can call. @@ -49,11 +53,20 @@ contract ContributionRouter { emit ClaimedFees(msg.sender, recipient, balance); } + function feePerMint() external view returns (uint96) { + return _storage.feePerMint; + } + + function caller() external view returns (address) { + return _storage.caller; + } + /// @notice Fallback function that forwards the call to the target contract /// and keeps the fee amount. The target contract is expected to /// be appended to the calldata. fallback() external payable { - uint256 feeAmount = feePerMint; + uint256 feeAmount = _storage.feePerMint; + _storage.caller = msg.sender; address target; assembly { target := shr(96, calldataload(sub(calldatasize(), 20))) @@ -72,4 +85,8 @@ contract ContributionRouter { emit ReceivedFees(msg.sender, feeAmount); } + + receive() external payable { + revert(); + } } From 20ba15e4228020603daede6ac9ddbe023d20a578 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Fri, 15 Sep 2023 16:05:15 -0400 Subject: [PATCH 02/10] gatekeepers mitigation --- contracts/gatekeepers/AllowListGateKeeper.sol | 11 ++++++++++- contracts/gatekeepers/TokenGateKeeper.sol | 9 +++++++++ deploy/Deploy.s.sol | 4 ++-- test/crowdfund/AuctionCrowdfund.t.sol | 4 ++-- test/crowdfund/BuyCrowdfund.t.sol | 6 +++--- test/crowdfund/Crowdfund.t.sol | 6 +++--- test/crowdfund/CrowdfundFactory.t.sol | 4 ++-- test/crowdfund/InitialETHCrowdfund.t.sol | 2 +- test/crowdfund/ReraiseETHCrowdfund.t.sol | 2 +- test/gatekeepers/AllowListGateKeeper.t.sol | 2 +- test/gatekeepers/TokenGateKeeper.t.sol | 2 +- 11 files changed, 35 insertions(+), 17 deletions(-) diff --git a/contracts/gatekeepers/AllowListGateKeeper.sol b/contracts/gatekeepers/AllowListGateKeeper.sol index 43ff7f7d..1c691118 100644 --- a/contracts/gatekeepers/AllowListGateKeeper.sol +++ b/contracts/gatekeepers/AllowListGateKeeper.sol @@ -3,19 +3,28 @@ pragma solidity 0.8.20; import "./IGateKeeper.sol"; import "openzeppelin/contracts/utils/cryptography/MerkleProof.sol"; +import { ContributionRouter } from "../crowdfund/ContributionRouter.sol"; /// @notice A gateKeeper that implements a simple allow list per gate. contract AllowListGateKeeper is IGateKeeper { + address public immutable CONTRIBUTION_ROUTER; uint96 private _lastId; - /// @notice Get the merkle root used by a gate identifyied by it's `id`. + /// @notice Get the merkle root used by a gate identified by it's `id`. mapping(uint96 => bytes32) public merkleRoots; + constructor(address contributionRouter) { + CONTRIBUTION_ROUTER = contributionRouter; + } + /// @inheritdoc IGateKeeper function isAllowed( address participant, bytes12 id, bytes memory userData ) external view returns (bool) { + if (participant == CONTRIBUTION_ROUTER) { + participant = ContributionRouter(payable(CONTRIBUTION_ROUTER)).caller(); + } bytes32[] memory proof = abi.decode(userData, (bytes32[])); bytes32 leaf; assembly { diff --git a/contracts/gatekeepers/TokenGateKeeper.sol b/contracts/gatekeepers/TokenGateKeeper.sol index 3e976cc8..2ac859ef 100644 --- a/contracts/gatekeepers/TokenGateKeeper.sol +++ b/contracts/gatekeepers/TokenGateKeeper.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.20; import { IGateKeeper } from "./IGateKeeper.sol"; +import { ContributionRouter } from "../crowdfund/ContributionRouter.sol"; /** * @notice Compatible with both ER20s and ERC721s. @@ -14,9 +15,14 @@ interface Token { * @notice a contract that implements an token gatekeeper */ contract TokenGateKeeper is IGateKeeper { + address public immutable CONTRIBUTION_ROUTER; // last gate id uint96 private _lastId; + constructor(address contributionRouter) { + CONTRIBUTION_ROUTER = contributionRouter; + } + struct TokenGate { Token token; uint256 minimumBalance; @@ -33,6 +39,9 @@ contract TokenGateKeeper is IGateKeeper { bytes12 id, bytes memory /* userData */ ) external view returns (bool) { + if (participant == CONTRIBUTION_ROUTER) { + participant = ContributionRouter(payable(CONTRIBUTION_ROUTER)).caller(); + } TokenGate memory _gate = gateInfo[uint96(id)]; return _gate.token.balanceOf(participant) >= _gate.minimumBalance; } diff --git a/deploy/Deploy.s.sol b/deploy/Deploy.s.sol index 2c49cf29..5d85bda3 100644 --- a/deploy/Deploy.s.sol +++ b/deploy/Deploy.s.sol @@ -360,7 +360,7 @@ abstract contract Deploy { console.log("### GateKeepers"); console.log(" Deploying - AllowListGateKeeper"); _trackDeployerGasBefore(); - allowListGateKeeper = new AllowListGateKeeper(); + allowListGateKeeper = new AllowListGateKeeper(address(0)); _trackDeployerGasAfter(); console.log(" Deployed - AllowListGateKeeper", address(allowListGateKeeper)); @@ -391,7 +391,7 @@ abstract contract Deploy { console.log(""); console.log(" Deploying - TokenGateKeeper"); _trackDeployerGasBefore(); - tokenGateKeeper = new TokenGateKeeper(); + tokenGateKeeper = new TokenGateKeeper(address(0)); _trackDeployerGasAfter(); console.log(" Deployed - TokenGateKeeper", address(tokenGateKeeper)); diff --git a/test/crowdfund/AuctionCrowdfund.t.sol b/test/crowdfund/AuctionCrowdfund.t.sol index a84c9655..cbd838c5 100644 --- a/test/crowdfund/AuctionCrowdfund.t.sol +++ b/test/crowdfund/AuctionCrowdfund.t.sol @@ -663,7 +663,7 @@ contract AuctionCrowdfundTest is Test, TestUtils { address contributor = _randomAddress(); // Create a AuctionCrowdfund instance with onlyHostCanBid and gatekeeper enabled. - AllowListGateKeeper gateKeeper = new AllowListGateKeeper(); + AllowListGateKeeper gateKeeper = new AllowListGateKeeper(address(0)); bytes32 contributorHash = keccak256(abi.encodePacked(contributor)); bytes12 gateKeeperId = gateKeeper.createGate(contributorHash); (uint256 auctionId, uint256 tokenId) = market.createAuction(0); @@ -710,7 +710,7 @@ contract AuctionCrowdfundTest is Test, TestUtils { address contributor = _randomAddress(); // Create a AuctionCrowdfund instance with a gatekeeper enabled. - AllowListGateKeeper gateKeeper = new AllowListGateKeeper(); + AllowListGateKeeper gateKeeper = new AllowListGateKeeper(address(0)); bytes32 contributorHash = keccak256(abi.encodePacked(contributor)); bytes12 gateKeeperId = gateKeeper.createGate(contributorHash); (uint256 auctionId, uint256 tokenId) = market.createAuction(0); diff --git a/test/crowdfund/BuyCrowdfund.t.sol b/test/crowdfund/BuyCrowdfund.t.sol index 4db9a1bd..c3e126be 100644 --- a/test/crowdfund/BuyCrowdfund.t.sol +++ b/test/crowdfund/BuyCrowdfund.t.sol @@ -288,7 +288,7 @@ contract BuyCrowdfundTest is Test, TestUtils { address contributor = _randomAddress(); // Create a BuyCrowdfund instance with a gatekeeper enabled. - AllowListGateKeeper gateKeeper = new AllowListGateKeeper(); + AllowListGateKeeper gateKeeper = new AllowListGateKeeper(address(0)); bytes32 contributorHash = keccak256(abi.encodePacked(contributor)); bytes12 gateKeeperId = gateKeeper.createGate(contributorHash); BuyCrowdfund cf = _createCrowdfund( @@ -328,7 +328,7 @@ contract BuyCrowdfundTest is Test, TestUtils { address contributor = _randomAddress(); // Create a BuyCrowdfund instance with onlyHostCanBuy and a gatekeeper enabled. - AllowListGateKeeper gateKeeper = new AllowListGateKeeper(); + AllowListGateKeeper gateKeeper = new AllowListGateKeeper(address(0)); bytes32 contributorHash = keccak256(abi.encodePacked(contributor)); bytes12 gateKeeperId = gateKeeper.createGate(contributorHash); BuyCrowdfund cf = _createCrowdfund( @@ -368,7 +368,7 @@ contract BuyCrowdfundTest is Test, TestUtils { address host = _randomAddress(); // Create a BuyCrowdfund instance with onlyHostCanBuy and a gatekeeper enabled. - AllowListGateKeeper gateKeeper = new AllowListGateKeeper(); + AllowListGateKeeper gateKeeper = new AllowListGateKeeper(address(0)); bytes12 gateKeeperId = gateKeeper.createGate(0); BuyCrowdfund cf = _createCrowdfund( 0, diff --git a/test/crowdfund/Crowdfund.t.sol b/test/crowdfund/Crowdfund.t.sol index a102e70c..a083b35c 100644 --- a/test/crowdfund/Crowdfund.t.sol +++ b/test/crowdfund/Crowdfund.t.sol @@ -794,7 +794,7 @@ contract CrowdfundTest is LintJSON, TestUtils { address payable contributor1 = _randomAddress(); address payable contributor2 = _randomAddress(); - AllowListGateKeeper gk = new AllowListGateKeeper(); + AllowListGateKeeper gk = new AllowListGateKeeper(address(0)); bytes12 gateId = gk.createGate(keccak256(abi.encodePacked(contributor1))); gateKeeper = gk; gateKeeperId = gateId; @@ -1014,7 +1014,7 @@ contract CrowdfundTest is LintJSON, TestUtils { function test_contributeFor_withGatekeeper_allowsSenderToContributeForOthers() external { address contributor = _randomAddress(); address recipient = _randomAddress(); - AllowListGateKeeper gk = new AllowListGateKeeper(); + AllowListGateKeeper gk = new AllowListGateKeeper(address(0)); bytes12 gateId = gk.createGate(keccak256(abi.encodePacked(contributor))); gateKeeper = gk; gateKeeperId = gateId; @@ -1038,7 +1038,7 @@ contract CrowdfundTest is LintJSON, TestUtils { function test_contributeFor_withGatekeeper_recipientNotBlockedFromChangingDelegate() external { address contributor = _randomAddress(); address recipient = _randomAddress(); - AllowListGateKeeper gk = new AllowListGateKeeper(); + AllowListGateKeeper gk = new AllowListGateKeeper(address(0)); bytes12 gateId = gk.createGate(keccak256(abi.encodePacked(contributor))); gateKeeper = gk; gateKeeperId = gateId; diff --git a/test/crowdfund/CrowdfundFactory.t.sol b/test/crowdfund/CrowdfundFactory.t.sol index 8ae3e064..ba2b3d05 100644 --- a/test/crowdfund/CrowdfundFactory.t.sol +++ b/test/crowdfund/CrowdfundFactory.t.sol @@ -33,8 +33,8 @@ contract CrowdfundFactoryTest is Test, TestUtils { new CollectionBatchBuyCrowdfund(globals); InitialETHCrowdfund initialETHCrowdfund = new InitialETHCrowdfund(globals); ReraiseETHCrowdfund reraiseETHCrowdfund = new ReraiseETHCrowdfund(globals); - AllowListGateKeeper allowListGateKeeper = new AllowListGateKeeper(); - TokenGateKeeper tokenGateKeeper = new TokenGateKeeper(); + AllowListGateKeeper allowListGateKeeper = new AllowListGateKeeper(address(0)); + TokenGateKeeper tokenGateKeeper = new TokenGateKeeper(address(0)); MetadataRegistry metadataRegistry = new MetadataRegistry(globals, _toAddressArray(address(partyFactory))); MetadataProvider metadataProvider = new MetadataProvider(globals); diff --git a/test/crowdfund/InitialETHCrowdfund.t.sol b/test/crowdfund/InitialETHCrowdfund.t.sol index 7e8533ce..c6d09eb2 100644 --- a/test/crowdfund/InitialETHCrowdfund.t.sol +++ b/test/crowdfund/InitialETHCrowdfund.t.sol @@ -691,7 +691,7 @@ contract InitialETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { address member = _randomAddress(); // Create allowlist gatekeeper with only member allowed - AllowListGateKeeper gatekeeper = new AllowListGateKeeper(); + AllowListGateKeeper gatekeeper = new AllowListGateKeeper(address(0)); bytes12 gateId = gatekeeper.createGate(keccak256(abi.encodePacked(member))); InitialETHCrowdfund crowdfund = _createCrowdfund( diff --git a/test/crowdfund/ReraiseETHCrowdfund.t.sol b/test/crowdfund/ReraiseETHCrowdfund.t.sol index 3746f210..a5cc1694 100644 --- a/test/crowdfund/ReraiseETHCrowdfund.t.sol +++ b/test/crowdfund/ReraiseETHCrowdfund.t.sol @@ -641,7 +641,7 @@ contract ReraiseETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { address member = _randomAddress(); // Create allowlist gatekeeper with only member allowed - AllowListGateKeeper gatekeeper = new AllowListGateKeeper(); + AllowListGateKeeper gatekeeper = new AllowListGateKeeper(address(0)); bytes12 gateId = gatekeeper.createGate(keccak256(abi.encodePacked(member))); bytes memory gateData = abi.encode(new bytes32[](0)); diff --git a/test/gatekeepers/AllowListGateKeeper.t.sol b/test/gatekeepers/AllowListGateKeeper.t.sol index f1214e13..3af19283 100644 --- a/test/gatekeepers/AllowListGateKeeper.t.sol +++ b/test/gatekeepers/AllowListGateKeeper.t.sol @@ -7,7 +7,7 @@ import "../../contracts/gatekeepers/AllowListGateKeeper.sol"; import "../TestUtils.sol"; contract AllowListGateKeeperTest is Test, TestUtils { - AllowListGateKeeper gk = new AllowListGateKeeper(); + AllowListGateKeeper gk = new AllowListGateKeeper(address(0)); // Generates a randomized 4-member allow list. function _randomAllowList() private view returns (address[4] memory allowList) { diff --git a/test/gatekeepers/TokenGateKeeper.t.sol b/test/gatekeepers/TokenGateKeeper.t.sol index 4352cb0d..6330dea8 100644 --- a/test/gatekeepers/TokenGateKeeper.t.sol +++ b/test/gatekeepers/TokenGateKeeper.t.sol @@ -17,7 +17,7 @@ contract TokenGateKeeperTest is Test, TestUtils { DummyERC721 dummyERC721 = new DummyERC721(); function setUp() public { - gk = new TokenGateKeeper(); + gk = new TokenGateKeeper(address(0)); } function testUniqueGateIds() public { From ec0a21dfe2874fb4558182dbd997d0522e6f8f8a Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 11:31:16 -0400 Subject: [PATCH 03/10] initial work on fixing --- contracts/crowdfund/Crowdfund.sol | 27 ++++---- contracts/crowdfund/InitialETHCrowdfund.sol | 41 ++++-------- test/crowdfund/Crowdfund.t.sol | 70 ++++++++++----------- test/crowdfund/InitialETHCrowdfund.t.sol | 3 +- 4 files changed, 62 insertions(+), 79 deletions(-) diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index c9d3c9ad..1d86bb98 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -366,24 +366,27 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT { /// @param initialDelegates The addresses to delegate to for each recipient. /// @param values The ETH to contribute for each recipient. /// @param gateDatas Data to pass to the gatekeeper to prove eligibility. - /// @param revertOnFailure If true, revert if any contribution fails. function batchContributeFor( address[] memory recipients, address[] memory initialDelegates, - uint256[] memory values, - bytes[] memory gateDatas, - bool revertOnFailure + uint96[] memory values, + bytes[] memory gateDatas ) external payable { + uint256 valuesSum; for (uint256 i; i < recipients.length; ++i) { - (bool s, bytes memory r) = address(this).call{ value: values[i] }( - abi.encodeCall( - this.contributeFor, - (recipients[i], initialDelegates[i], gateDatas[i]) - ) + _setDelegate(recipients[i], initialDelegates[i]); + + _contribute( + recipients[i], + initialDelegates[i], + values[i], + totalContributions, + gateDatas[i] ); - if (revertOnFailure && !s) { - r.rawRevert(); - } + valuesSum += values[i]; + } + if (msg.value != valuesSum) { + revert(); } } diff --git a/contracts/crowdfund/InitialETHCrowdfund.sol b/contracts/crowdfund/InitialETHCrowdfund.sol index 29729b2b..10c0fc7e 100644 --- a/contracts/crowdfund/InitialETHCrowdfund.sol +++ b/contracts/crowdfund/InitialETHCrowdfund.sol @@ -89,8 +89,6 @@ contract InitialETHCrowdfund is ETHCrowdfundBase { // The data required to be validated by the `gatekeeper`, if set. If no // `gatekeeper` is set, this can be empty. bytes[] gateDatas; - // Whether to revert if any individual contribution fails or continue. - bool revertOnFailure; } event Refunded(address indexed contributor, uint256 indexed tokenId, uint256 amount); @@ -255,35 +253,20 @@ contract InitialETHCrowdfund is ETHCrowdfundBase { function batchContributeFor( BatchContributeForArgs calldata args ) external payable onlyDelegateCall returns (uint96[] memory votingPowers) { - uint256 numContributions = args.recipients.length; - votingPowers = new uint96[](numContributions); - - uint256 ethAvailable = msg.value; - for (uint256 i; i < numContributions; ++i) { - (bool s, bytes memory r) = address(this).call{ value: args.values[i] }( - abi.encodeCall( - this.contributeFor, - ( - args.tokenIds[i], - args.recipients[i], - args.initialDelegates[i], - args.gateDatas[i] - ) - ) + uint256 valuesSum; + for (uint256 i; i < args.recipients.length; ++i) { + _contribute( + args.recipients[i], + args.initialDelegates[i], + args.values[i], + args.tokenIds[i], + args.gateDatas[i] ); - - if (!s) { - if (args.revertOnFailure) { - r.rawRevert(); - } - } else { - votingPowers[i] = abi.decode(r, (uint96)); - ethAvailable -= args.values[i]; - } + valuesSum += args.values[i]; + } + if (msg.value != valuesSum) { + revert(); } - - // Refund any unused ETH. - if (ethAvailable > 0) payable(msg.sender).transfer(ethAvailable); } function _contribute( diff --git a/test/crowdfund/Crowdfund.t.sol b/test/crowdfund/Crowdfund.t.sol index a083b35c..3b437314 100644 --- a/test/crowdfund/Crowdfund.t.sol +++ b/test/crowdfund/Crowdfund.t.sol @@ -1060,7 +1060,7 @@ contract CrowdfundTest is LintJSON, TestUtils { address contributor = _randomAddress(); address[] memory recipients = new address[](3); address[] memory initialDelegates = new address[](3); - uint256[] memory values = new uint256[](3); + uint96[] memory values = new uint96[](3); bytes[] memory gateDatas = new bytes[](3); for (uint256 i; i < 3; ++i) { recipients[i] = _randomAddress(); @@ -1075,8 +1075,7 @@ contract CrowdfundTest is LintJSON, TestUtils { recipients, initialDelegates, values, - gateDatas, - true + gateDatas ); for (uint256 i; i < 3; ++i) { assertEq(cf.getContributionEntriesByContributorCount(contributor), 0); @@ -1091,39 +1090,38 @@ contract CrowdfundTest is LintJSON, TestUtils { } } - function test_batchContributeFor_doesNotRevertOnFailure() external { - TestableCrowdfund cf = _createCrowdfund(0); - address contributor = _randomAddress(); - address[] memory recipients = new address[](4); - address[] memory initialDelegates = new address[](4); - uint256[] memory values = new uint256[](4); - bytes[] memory gateDatas = new bytes[](4); - for (uint256 i; i < 3; ++i) { - recipients[i] = _randomAddress(); - initialDelegates[i] = _randomAddress(); - values[i] = 1e18; - gateDatas[i] = ""; - } - vm.deal(contributor, 3e18); - vm.prank(contributor); - // Contributor contributes on recipient's behalf and expect fail - vm.expectRevert(Crowdfund.InvalidDelegateError.selector); - cf.batchContributeFor{ value: contributor.balance }( - recipients, - initialDelegates, - values, - gateDatas, - true - ); - // Contributor contributes on recipient's behalf and do not revert on fail - cf.batchContributeFor{ value: contributor.balance }( - recipients, - initialDelegates, - values, - gateDatas, - false - ); - } + // function test_batchContributeFor_doesNotRevertOnFailure() external { + // TestableCrowdfund cf = _createCrowdfund(0); + // address contributor = _randomAddress(); + // address[] memory recipients = new address[](4); + // address[] memory initialDelegates = new address[](4); + // uint96[] memory values = new uint96[](4); + // bytes[] memory gateDatas = new bytes[](4); + // for (uint256 i; i < 3; ++i) { + // recipients[i] = _randomAddress(); + // initialDelegates[i] = _randomAddress(); + // values[i] = 1e18; + // gateDatas[i] = ""; + // } + // vm.deal(contributor, 3e18); + // vm.prank(contributor); + // // Contributor contributes on recipient's behalf and expect fail + // vm.expectRevert(Crowdfund.InvalidDelegateError.selector); + // cf.batchContributeFor{ value: contributor.balance }( + // recipients, + // initialDelegates, + // values, + // gateDatas + // ); + // // Contributor contributes on recipient's behalf and do not revert on fail + // cf.batchContributeFor{ value: contributor.balance }( + // recipients, + // initialDelegates, + // values, + // gateDatas, + // false + // ); + // } function test_canReuseContributionEntry() external { TestableCrowdfund cf = _createCrowdfund(0); diff --git a/test/crowdfund/InitialETHCrowdfund.t.sol b/test/crowdfund/InitialETHCrowdfund.t.sol index c6d09eb2..3a228d12 100644 --- a/test/crowdfund/InitialETHCrowdfund.t.sol +++ b/test/crowdfund/InitialETHCrowdfund.t.sol @@ -990,8 +990,7 @@ contract InitialETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { recipients: recipients, initialDelegates: delegates, values: values, - gateDatas: gateDatas, - revertOnFailure: false + gateDatas: gateDatas }) ); From 66f6e195fd17b2de9ac37803451ce20a95595a2c Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 14:23:58 -0400 Subject: [PATCH 04/10] fix tests --- contracts/crowdfund/ContributionRouter.sol | 2 +- contracts/crowdfund/Crowdfund.sol | 3 ++- contracts/crowdfund/ETHCrowdfundBase.sol | 1 + contracts/crowdfund/InitialETHCrowdfund.sol | 5 +++-- .../ContributionRouterIntegration.t.sol | 3 +-- test/crowdfund/InitialETHCrowdfund.t.sol | 18 +++++++++--------- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/contracts/crowdfund/ContributionRouter.sol b/contracts/crowdfund/ContributionRouter.sol index e6608869..c2adfd1a 100644 --- a/contracts/crowdfund/ContributionRouter.sol +++ b/contracts/crowdfund/ContributionRouter.sol @@ -76,7 +76,7 @@ contract ContributionRouter { assembly { // 228 is the offset of the length of `tokenIds` in the // calldata. - numOfMints := calldataload(228) + numOfMints := calldataload(196) } feeAmount *= numOfMints; } diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 1d86bb98..82a8ef0a 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -110,6 +110,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT { error OnlyWhenEmergencyActionsAllowedError(); error BelowMinimumContributionsError(uint96 contributions, uint96 minContributions); error AboveMaximumContributionsError(uint96 contributions, uint96 maxContributions); + error InvalidMessageValue(); event Burned(address contributor, uint256 ethUsed, uint256 ethOwed, uint256 votingPower); event Contributed( @@ -386,7 +387,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT { valuesSum += values[i]; } if (msg.value != valuesSum) { - revert(); + revert InvalidMessageValue(); } } diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol index 2cb7a0d7..db0e78d6 100644 --- a/contracts/crowdfund/ETHCrowdfundBase.sol +++ b/contracts/crowdfund/ETHCrowdfundBase.sol @@ -70,6 +70,7 @@ contract ETHCrowdfundBase is Implementation { error ZeroVotingPowerError(); error FundingSplitAlreadyPaidError(); error FundingSplitNotConfiguredError(); + error InvalidMessageValue(); event Contributed( address indexed sender, diff --git a/contracts/crowdfund/InitialETHCrowdfund.sol b/contracts/crowdfund/InitialETHCrowdfund.sol index 10c0fc7e..a5d21d89 100644 --- a/contracts/crowdfund/InitialETHCrowdfund.sol +++ b/contracts/crowdfund/InitialETHCrowdfund.sol @@ -253,9 +253,10 @@ contract InitialETHCrowdfund is ETHCrowdfundBase { function batchContributeFor( BatchContributeForArgs calldata args ) external payable onlyDelegateCall returns (uint96[] memory votingPowers) { + votingPowers = new uint96[](args.recipients.length); uint256 valuesSum; for (uint256 i; i < args.recipients.length; ++i) { - _contribute( + votingPowers[i] = _contribute( args.recipients[i], args.initialDelegates[i], args.values[i], @@ -265,7 +266,7 @@ contract InitialETHCrowdfund is ETHCrowdfundBase { valuesSum += args.values[i]; } if (msg.value != valuesSum) { - revert(); + revert InvalidMessageValue(); } } diff --git a/test/crowdfund/ContributionRouterIntegration.t.sol b/test/crowdfund/ContributionRouterIntegration.t.sol index dff9243f..fd8f15b1 100644 --- a/test/crowdfund/ContributionRouterIntegration.t.sol +++ b/test/crowdfund/ContributionRouterIntegration.t.sol @@ -141,8 +141,7 @@ contract ContributionRouterIntegrationTest is TestUtils { recipients: recipients, initialDelegates: delegates, values: values, - gateDatas: gateDatas, - revertOnFailure: true + gateDatas: gateDatas }) ) ); diff --git a/test/crowdfund/InitialETHCrowdfund.t.sol b/test/crowdfund/InitialETHCrowdfund.t.sol index 3a228d12..832b895e 100644 --- a/test/crowdfund/InitialETHCrowdfund.t.sol +++ b/test/crowdfund/InitialETHCrowdfund.t.sol @@ -974,17 +974,17 @@ contract InitialETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { // Batch contribute for vm.prank(sender); - uint256[] memory tokenIds = new uint256[](4); - address payable[] memory recipients = new address payable[](4); - address[] memory delegates = new address[](4); - uint96[] memory values = new uint96[](4); - bytes[] memory gateDatas = new bytes[](4); - for (uint256 i; i < 4; ++i) { + uint256[] memory tokenIds = new uint256[](3); + address payable[] memory recipients = new address payable[](3); + address[] memory delegates = new address[](3); + uint96[] memory values = new uint96[](3); + bytes[] memory gateDatas = new bytes[](3); + for (uint256 i; i < 3; ++i) { recipients[i] = _randomAddress(); delegates[i] = _randomAddress(); - values[i] = i != 3 ? 1 ether : 0.5 ether; // Last contribution is below min contribution + values[i] = 1 ether; } - uint96[] memory votingPowers = crowdfund.batchContributeFor{ value: 4 ether }( + uint96[] memory votingPowers = crowdfund.batchContributeFor{ value: 3 ether }( InitialETHCrowdfund.BatchContributeForArgs({ tokenIds: tokenIds, recipients: recipients, @@ -994,7 +994,7 @@ contract InitialETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { }) ); - assertEq(address(sender).balance, 1 ether); // Should be refunded 1 ETH + assertEq(address(sender).balance, 1 ether); for (uint256 i; i < 3; ++i) { assertEq(votingPowers[i], 1 ether); assertEq(crowdfund.delegationsByContributor(recipients[i]), delegates[i]); From bb02d22f21dcbe82d295b1bc69d9f7e909b64e8f Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 14:37:26 -0400 Subject: [PATCH 05/10] add invalid message value tests --- test/crowdfund/Crowdfund.t.sol | 56 ++++++++++-------------- test/crowdfund/InitialETHCrowdfund.t.sol | 48 ++++++++++++++++++++ 2 files changed, 72 insertions(+), 32 deletions(-) diff --git a/test/crowdfund/Crowdfund.t.sol b/test/crowdfund/Crowdfund.t.sol index 3b437314..cebac093 100644 --- a/test/crowdfund/Crowdfund.t.sol +++ b/test/crowdfund/Crowdfund.t.sol @@ -1090,38 +1090,30 @@ contract CrowdfundTest is LintJSON, TestUtils { } } - // function test_batchContributeFor_doesNotRevertOnFailure() external { - // TestableCrowdfund cf = _createCrowdfund(0); - // address contributor = _randomAddress(); - // address[] memory recipients = new address[](4); - // address[] memory initialDelegates = new address[](4); - // uint96[] memory values = new uint96[](4); - // bytes[] memory gateDatas = new bytes[](4); - // for (uint256 i; i < 3; ++i) { - // recipients[i] = _randomAddress(); - // initialDelegates[i] = _randomAddress(); - // values[i] = 1e18; - // gateDatas[i] = ""; - // } - // vm.deal(contributor, 3e18); - // vm.prank(contributor); - // // Contributor contributes on recipient's behalf and expect fail - // vm.expectRevert(Crowdfund.InvalidDelegateError.selector); - // cf.batchContributeFor{ value: contributor.balance }( - // recipients, - // initialDelegates, - // values, - // gateDatas - // ); - // // Contributor contributes on recipient's behalf and do not revert on fail - // cf.batchContributeFor{ value: contributor.balance }( - // recipients, - // initialDelegates, - // values, - // gateDatas, - // false - // ); - // } + function test_batchContributeFor_invalidMessageValue() external { + TestableCrowdfund cf = _createCrowdfund(0); + address contributor = _randomAddress(); + address[] memory recipients = new address[](3); + address[] memory initialDelegates = new address[](3); + uint96[] memory values = new uint96[](3); + bytes[] memory gateDatas = new bytes[](3); + for (uint256 i; i < 3; ++i) { + recipients[i] = _randomAddress(); + initialDelegates[i] = _randomAddress(); + values[i] = 1e18; + gateDatas[i] = ""; + } + // Contributor contributes on recipient's behalf + vm.deal(contributor, 3e18); + vm.prank(contributor); + vm.expectRevert(Crowdfund.InvalidMessageValue.selector); + cf.batchContributeFor{ value: contributor.balance - 100 }( + recipients, + initialDelegates, + values, + gateDatas + ); + } function test_canReuseContributionEntry() external { TestableCrowdfund cf = _createCrowdfund(0); diff --git a/test/crowdfund/InitialETHCrowdfund.t.sol b/test/crowdfund/InitialETHCrowdfund.t.sol index 832b895e..960648bf 100644 --- a/test/crowdfund/InitialETHCrowdfund.t.sol +++ b/test/crowdfund/InitialETHCrowdfund.t.sol @@ -1007,6 +1007,54 @@ contract InitialETHCrowdfundTest is LintJSON, TestUtils, ERC721Receiver { assertEq(party.votingPowerByTokenId(4), 0); } + function test_batchContributeFor_works_invalidMessageValue() public { + InitialETHCrowdfund crowdfund = _createCrowdfund( + CreateCrowdfundArgs({ + initialContribution: 0, + initialContributor: payable(address(0)), + initialDelegate: address(0), + minContributions: 1 ether, + maxContributions: type(uint96).max, + disableContributingForExistingCard: false, + minTotalContributions: 3 ether, + maxTotalContributions: 5 ether, + duration: 7 days, + exchangeRateBps: 1e4, + fundingSplitBps: 0, + fundingSplitRecipient: payable(address(0)), + gateKeeper: IGateKeeper(address(0)), + gateKeeperId: bytes12(0) + }) + ); + Party party = crowdfund.party(); + + address sender = _randomAddress(); + vm.deal(sender, 4 ether); + + // Batch contribute for + uint256[] memory tokenIds = new uint256[](3); + address payable[] memory recipients = new address payable[](3); + address[] memory delegates = new address[](3); + uint96[] memory values = new uint96[](3); + bytes[] memory gateDatas = new bytes[](3); + for (uint256 i; i < 3; ++i) { + recipients[i] = _randomAddress(); + delegates[i] = _randomAddress(); + values[i] = 1 ether; + } + vm.expectRevert(ETHCrowdfundBase.InvalidMessageValue.selector); + vm.prank(sender); + uint96[] memory votingPowers = crowdfund.batchContributeFor{ value: 3 ether - 100 }( + InitialETHCrowdfund.BatchContributeForArgs({ + tokenIds: tokenIds, + recipients: recipients, + initialDelegates: delegates, + values: values, + gateDatas: gateDatas + }) + ); + } + function test_finalize_works() public { InitialETHCrowdfund crowdfund = _createCrowdfund( CreateCrowdfundArgs({ From 1fa487a005c86fb33c7272c2c1e6ed8160c525d6 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:04:17 -0400 Subject: [PATCH 06/10] test gatekeeper --- .../ContributionRouterIntegration.t.sol | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/crowdfund/ContributionRouterIntegration.t.sol b/test/crowdfund/ContributionRouterIntegration.t.sol index fd8f15b1..94336f6a 100644 --- a/test/crowdfund/ContributionRouterIntegration.t.sol +++ b/test/crowdfund/ContributionRouterIntegration.t.sol @@ -6,15 +6,20 @@ import "../../contracts/party/PartyFactory.sol"; import "../../contracts/crowdfund/InitialETHCrowdfund.sol"; import "../../contracts/crowdfund/ContributionRouter.sol"; import "./TestableCrowdfund.sol"; +import { TokenGateKeeper, Token } from "../../contracts/gatekeepers/TokenGateKeeper.sol"; +import { DummyERC20 } from "../DummyERC20.sol"; import "../TestUtils.sol"; contract ContributionRouterIntegrationTest is TestUtils { InitialETHCrowdfund ethCrowdfund; + InitialETHCrowdfund gatekeeperEthCrowdfund; TestableCrowdfund nftCrowdfund; Globals globals; Party partyImpl; PartyFactory partyFactory; + TokenGateKeeper gateKeeper; + DummyERC20 gatekeepToken; uint96 feePerMint; ContributionRouter router; @@ -27,6 +32,10 @@ contract ContributionRouterIntegrationTest is TestUtils { partyImpl = new Party(globals); partyFactory = new PartyFactory(globals); + gateKeeper = new TokenGateKeeper(address(router)); + gatekeepToken = new DummyERC20(); + bytes12 gateKeeperId = gateKeeper.createGate(Token(address(gatekeepToken)), 100); + InitialETHCrowdfund initialETHCrowdfundImpl = new InitialETHCrowdfund(globals); InitialETHCrowdfund.InitialETHCrowdfundOptions memory ethCrowdfundOpts; @@ -58,6 +67,20 @@ contract ContributionRouterIntegrationTest is TestUtils { ) ); + ethCrowdfundOpts.gateKeeper = gateKeeper; + ethCrowdfundOpts.gateKeeperId = gateKeeperId; + gatekeeperEthCrowdfund = InitialETHCrowdfund( + payable( + new Proxy( + initialETHCrowdfundImpl, + abi.encodeCall( + InitialETHCrowdfund.initialize, + (ethCrowdfundOpts, partyOpts, MetadataProvider(address(0)), "") + ) + ) + ) + ); + Crowdfund.CrowdfundOptions memory nftCrowdfundOpts; nftCrowdfundOpts.name = "Test Party"; nftCrowdfundOpts.symbol = "TEST"; @@ -97,6 +120,48 @@ contract ContributionRouterIntegrationTest is TestUtils { assertEq(member.balance, 0); } + function test_contributionFee_ethCrowdfund_gatekeeper() public { + // Setup for contribution. + address payable member = _randomAddress(); + uint256 amount = 1 ether; + vm.deal(member, amount); + bytes memory data = abi.encodeCall( + InitialETHCrowdfund.contributeFor, + (0, member, member, "") + ); + + vm.prank(member); + (bool success, bytes memory res) = address(router).call{ value: amount }( + abi.encodePacked(data, gatekeeperEthCrowdfund) + ); + + // Reverted by gatekeeper + assertEq(success, false); + assertEq( + res, + abi.encodeWithSelector( + ETHCrowdfundBase.NotAllowedByGateKeeperError.selector, + member, + address(gateKeeper), + bytes12(uint96(1)), + "" + ) + ); + + gatekeepToken.deal(member, 200); + vm.prank(member); + // Recall with tokens now + (success, res) = address(router).call{ value: amount }( + abi.encodePacked(data, gatekeeperEthCrowdfund) + ); + + assertEq(success, true); + assertEq(res.length, 0); + assertEq(address(gatekeeperEthCrowdfund).balance, amount - feePerMint); + assertEq(address(router).balance, feePerMint); + assertEq(member.balance, 0); + } + function test_contributionFee_ethCrowdfund_revert() public { // Setup for contribution. address payable member = _randomAddress(); From 16b7d4b8c9e5af48a9141345b1c25cfdcef584a2 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:28:08 -0400 Subject: [PATCH 07/10] fix gatekeep revert --- contracts/crowdfund/InitialETHCrowdfund.sol | 7 +------ test/crowdfund/ContributionRouterIntegration.t.sol | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/contracts/crowdfund/InitialETHCrowdfund.sol b/contracts/crowdfund/InitialETHCrowdfund.sol index a5d21d89..298537a2 100644 --- a/contracts/crowdfund/InitialETHCrowdfund.sol +++ b/contracts/crowdfund/InitialETHCrowdfund.sol @@ -286,12 +286,7 @@ contract InitialETHCrowdfund is ETHCrowdfundBase { IGateKeeper _gateKeeper = gateKeeper; if (_gateKeeper != IGateKeeper(address(0))) { if (!_gateKeeper.isAllowed(msg.sender, gateKeeperId, gateData)) { - revert NotAllowedByGateKeeperError( - contributor, - _gateKeeper, - gateKeeperId, - gateData - ); + revert NotAllowedByGateKeeperError(msg.sender, _gateKeeper, gateKeeperId, gateData); } } diff --git a/test/crowdfund/ContributionRouterIntegration.t.sol b/test/crowdfund/ContributionRouterIntegration.t.sol index 94336f6a..a1007aad 100644 --- a/test/crowdfund/ContributionRouterIntegration.t.sol +++ b/test/crowdfund/ContributionRouterIntegration.t.sol @@ -141,7 +141,7 @@ contract ContributionRouterIntegrationTest is TestUtils { res, abi.encodeWithSelector( ETHCrowdfundBase.NotAllowedByGateKeeperError.selector, - member, + address(router), address(gateKeeper), bytes12(uint96(1)), "" From f7c5190a07a64e1b9dbe12be42f07b7f4bc3cc03 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:43:39 -0400 Subject: [PATCH 08/10] add docs --- contracts/crowdfund/ContributionRouter.sol | 3 +++ contracts/gatekeepers/AllowListGateKeeper.sol | 1 + contracts/gatekeepers/TokenGateKeeper.sol | 1 + 3 files changed, 5 insertions(+) diff --git a/contracts/crowdfund/ContributionRouter.sol b/contracts/crowdfund/ContributionRouter.sol index c2adfd1a..33f3cc02 100644 --- a/contracts/crowdfund/ContributionRouter.sol +++ b/contracts/crowdfund/ContributionRouter.sol @@ -53,10 +53,12 @@ contract ContributionRouter { emit ClaimedFees(msg.sender, recipient, balance); } + /// @notice View function to get the `feePerMint` value. function feePerMint() external view returns (uint96) { return _storage.feePerMint; } + /// @notice View function to get the most recent caller to the contract. function caller() external view returns (address) { return _storage.caller; } @@ -86,6 +88,7 @@ contract ContributionRouter { emit ReceivedFees(msg.sender, feeAmount); } + // Revert for any vanilla ETH transfers. receive() external payable { revert(); } diff --git a/contracts/gatekeepers/AllowListGateKeeper.sol b/contracts/gatekeepers/AllowListGateKeeper.sol index 1c691118..3d60b4b5 100644 --- a/contracts/gatekeepers/AllowListGateKeeper.sol +++ b/contracts/gatekeepers/AllowListGateKeeper.sol @@ -7,6 +7,7 @@ import { ContributionRouter } from "../crowdfund/ContributionRouter.sol"; /// @notice A gateKeeper that implements a simple allow list per gate. contract AllowListGateKeeper is IGateKeeper { + /// @notice The address of the canonical contribution router. address public immutable CONTRIBUTION_ROUTER; uint96 private _lastId; /// @notice Get the merkle root used by a gate identified by it's `id`. diff --git a/contracts/gatekeepers/TokenGateKeeper.sol b/contracts/gatekeepers/TokenGateKeeper.sol index 2ac859ef..48945e0f 100644 --- a/contracts/gatekeepers/TokenGateKeeper.sol +++ b/contracts/gatekeepers/TokenGateKeeper.sol @@ -15,6 +15,7 @@ interface Token { * @notice a contract that implements an token gatekeeper */ contract TokenGateKeeper is IGateKeeper { + /// @notice The address of the canonical contribution router. address public immutable CONTRIBUTION_ROUTER; // last gate id uint96 private _lastId; From 4435d9c96a3dca2a700cd6f7c16fb1cc74342b9a Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:55:11 -0400 Subject: [PATCH 09/10] Update comment contracts/crowdfund/ContributionRouter.sol Co-authored-by: Brian Le --- contracts/crowdfund/ContributionRouter.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/crowdfund/ContributionRouter.sol b/contracts/crowdfund/ContributionRouter.sol index 33f3cc02..709578e7 100644 --- a/contracts/crowdfund/ContributionRouter.sol +++ b/contracts/crowdfund/ContributionRouter.sol @@ -76,7 +76,7 @@ contract ContributionRouter { if (msg.sig == InitialETHCrowdfund.batchContributeFor.selector) { uint256 numOfMints; assembly { - // 228 is the offset of the length of `tokenIds` in the + // 196 is the offset of the length of `tokenIds` in the // calldata. numOfMints := calldataload(196) } From 40b808922608891063475d4f7c494e7bfdc97d63 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 18 Sep 2023 17:01:24 -0400 Subject: [PATCH 10/10] add delegate note --- contracts/crowdfund/ContributionRouter.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/crowdfund/ContributionRouter.sol b/contracts/crowdfund/ContributionRouter.sol index 709578e7..151e6a10 100644 --- a/contracts/crowdfund/ContributionRouter.sol +++ b/contracts/crowdfund/ContributionRouter.sol @@ -66,6 +66,8 @@ contract ContributionRouter { /// @notice Fallback function that forwards the call to the target contract /// and keeps the fee amount. The target contract is expected to /// be appended to the calldata. + /// @dev Only initial contributions per address allow for setting the delegate. + /// Use the `delegate` function to set afterwards. fallback() external payable { uint256 feeAmount = _storage.feePerMint; _storage.caller = msg.sender;