From f3af0ed35a77bb47599a7fbc76200c9c4907c653 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 25 Oct 2022 09:06:31 -0700 Subject: [PATCH] do not allow arbitrary calls to `Zora.cancelAuction()` unless it is the last call in sequence (#154) --- .../proposals/ArbitraryCallsProposal.sol | 29 +++++++++-- .../proposals/ProposalExecutionEngine.sol | 1 + sol-tests/crowdfund/Crowdfund.t.sol | 2 - .../proposals/ArbitraryCallsProposal.t.sol | 49 ++++++++++++++++++- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/contracts/proposals/ArbitraryCallsProposal.sol b/contracts/proposals/ArbitraryCallsProposal.sol index 8ced652b..e4668e56 100644 --- a/contracts/proposals/ArbitraryCallsProposal.sol +++ b/contracts/proposals/ArbitraryCallsProposal.sol @@ -7,6 +7,7 @@ import "../tokens/IERC721Receiver.sol"; import "../tokens/ERC1155Receiver.sol"; import "../utils/LibSafeERC721.sol"; import "../utils/LibAddress.sol"; +import "../vendor/markets/IZoraAuctionHouse.sol"; import "./vendor/IOpenseaExchange.sol"; import "./LibProposal.sol"; @@ -39,6 +40,12 @@ contract ArbitraryCallsProposal { event ArbitraryCallExecuted(uint256 proposalId, uint256 idx, uint256 count); + IZoraAuctionHouse private immutable _ZORA; + + constructor(IZoraAuctionHouse zora) { + _ZORA = zora; + } + function _executeArbitraryCalls( IProposalExecutionEngine.ExecuteProposalParams memory params ) @@ -67,7 +74,7 @@ contract ArbitraryCallsProposal { // Execute an arbitrary call. _executeSingleArbitraryCall( i, - calls[i], + calls, params.preciousTokens, params.preciousTokenIds, isUnanimous, @@ -101,7 +108,7 @@ contract ArbitraryCallsProposal { function _executeSingleArbitraryCall( uint256 idx, - ArbitraryCall memory call, + ArbitraryCall[] memory calls, IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds, bool isUnanimous, @@ -109,8 +116,16 @@ contract ArbitraryCallsProposal { ) private { + ArbitraryCall memory call = calls[idx]; // Check that the call is not prohibited. - if (!_isCallAllowed(call, isUnanimous, preciousTokens, preciousTokenIds)) { + if (!_isCallAllowed( + call, + isUnanimous, + idx, + calls.length, + preciousTokens, + preciousTokenIds)) + { revert CallProhibitedError(call.target, call.data); } // Check that we have enough ETH to execute the call. @@ -151,6 +166,8 @@ contract ArbitraryCallsProposal { function _isCallAllowed( ArbitraryCall memory call, bool isUnanimous, + uint256 callIndex, + uint256 callsCount, IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds ) @@ -197,6 +214,12 @@ contract ArbitraryCallsProposal { if (isApproved) { return !LibProposal.isTokenPrecious(IERC721(call.target), preciousTokens); } + // Can only call cancelAuction on the zora AH if it's the last call + // in the sequence. + } else if (selector == IZoraAuctionHouse.cancelAuction.selector) { + if (call.target == address(_ZORA)) { + return callIndex + 1 == callsCount; + } } } // Can never call receive hooks on any target. diff --git a/contracts/proposals/ProposalExecutionEngine.sol b/contracts/proposals/ProposalExecutionEngine.sol index 5be882da..71e4c774 100644 --- a/contracts/proposals/ProposalExecutionEngine.sol +++ b/contracts/proposals/ProposalExecutionEngine.sol @@ -88,6 +88,7 @@ contract ProposalExecutionEngine is ListOnOpenseaProposal(globals, seaport, seaportConduitController) ListOnZoraProposal(globals, zoraAuctionHouse) FractionalizeProposal(fractionalVaultFactory) + ArbitraryCallsProposal(zoraAuctionHouse) { _GLOBALS = globals; } diff --git a/sol-tests/crowdfund/Crowdfund.t.sol b/sol-tests/crowdfund/Crowdfund.t.sol index a1515217..32b34e87 100644 --- a/sol-tests/crowdfund/Crowdfund.t.sol +++ b/sol-tests/crowdfund/Crowdfund.t.sol @@ -1022,7 +1022,6 @@ contract CrowdfundTest is Test, TestUtils { function test_canReuseContributionEntry() external { TestableCrowdfund cf = _createCrowdfund(0); address contributor = _randomAddress(); - address payable badERC721Receiver = payable(new BadERC721Receiver()); // Contributor contributes twice back-to-back. vm.deal(contributor, 3); vm.prank(contributor); @@ -1039,7 +1038,6 @@ contract CrowdfundTest is Test, TestUtils { TestableCrowdfund cf = _createCrowdfund(0); address contributor1 = _randomAddress(); address contributor2 = _randomAddress(); - address payable badERC721Receiver = payable(new BadERC721Receiver()); // contributor1 sandwiches contributor2. vm.deal(contributor1, 3); vm.deal(contributor2, 10); diff --git a/sol-tests/proposals/ArbitraryCallsProposal.t.sol b/sol-tests/proposals/ArbitraryCallsProposal.t.sol index 2653c958..c64d3001 100644 --- a/sol-tests/proposals/ArbitraryCallsProposal.t.sol +++ b/sol-tests/proposals/ArbitraryCallsProposal.t.sol @@ -11,6 +11,9 @@ import "../TestUtils.sol"; import "../DummyERC721.sol"; contract TestableArbitraryCallsProposal is ArbitraryCallsProposal { + + constructor(IZoraAuctionHouse zora) ArbitraryCallsProposal(zora) {} + function execute( IProposalExecutionEngine.ExecuteProposalParams calldata params ) @@ -66,6 +69,10 @@ contract ArbitraryCallTarget { } } +contract FakeZora { + function cancelAuction(uint256 auctionId) external {} +} + contract ArbitraryCallsProposalTest is Test, TestUtils, @@ -80,7 +87,9 @@ contract ArbitraryCallsProposalTest is event ArbitraryCallExecuted(uint256 proposalId, uint256 idx, uint256 count); ArbitraryCallTarget target = new ArbitraryCallTarget(); - TestableArbitraryCallsProposal testContract = new TestableArbitraryCallsProposal(); + FakeZora zora = new FakeZora(); + TestableArbitraryCallsProposal testContract = + new TestableArbitraryCallsProposal(IZoraAuctionHouse(address(zora))); IERC721[] preciousTokens; uint256[] preciousTokenIds; @@ -615,6 +624,44 @@ contract ArbitraryCallsProposalTest is testContract.execute(prop); } + function test_cannotCallCancelAuctionOnZoraIfNotLastCall() external { + ( + ArbitraryCallsProposal.ArbitraryCall[] memory calls, + ) = _createSimpleCalls(2, false); + calls[0].target = payable(address(zora)); + calls[0].data = abi.encodeCall( + IZoraAuctionHouse.cancelAuction, + (_randomUint256()) // params don't matter + ); + IProposalExecutionEngine.ExecuteProposalParams memory prop = + _createTestProposal(calls); + vm.expectRevert(abi.encodeWithSelector( + ArbitraryCallsProposal.CallProhibitedError.selector, + calls[0].target, + calls[0].data + )); + testContract.execute(prop); + } + + function test_canCallCancelAuctionOnZoraIfLastCall() external { + ( + ArbitraryCallsProposal.ArbitraryCall[] memory calls, + ) = _createSimpleCalls(2, false); + calls[1].target = payable(address(zora)); + calls[1].data = abi.encodeCall( + IZoraAuctionHouse.cancelAuction, + (_randomUint256()) // params don't matter + ); + IProposalExecutionEngine.ExecuteProposalParams memory prop = + _createTestProposal(calls); + _expectEmit0(); + emit ArbitraryCallExecuted(prop.proposalId, 0, 2); + _expectEmit0(); + emit ArbitraryCallExecuted(prop.proposalId, 1, 2); + testContract.execute(prop); + } + + function test_cannotExecuteShortApproveCallData() external { ( ArbitraryCallsProposal.ArbitraryCall[] memory calls,