From 973c85b833961f37cf00b4918b109184599dd260 Mon Sep 17 00:00:00 2001 From: James Duncombe Date: Wed, 1 Nov 2023 17:57:00 +0000 Subject: [PATCH] wip --- contracts/common/AHasAutomatons.sol | 2 +- contracts/common/AHasForwarder.sol | 43 +++++++++++++++++-- contracts/common/AHasGovernors.sol | 2 +- contracts/common/AHasMembers.sol | 8 ++-- contracts/fast/FastAccessFacet.sol | 15 ++++++- contracts/interfaces/ICustomErrors.sol | 1 + contracts/issuer/IssuerAccessFacet.sol | 16 ++++++- .../marketplace/MarketplaceAccessFacet.sol | 24 ++++++++--- .../MarketplaceAccessFacet.test.ts | 28 ++++++++++-- 9 files changed, 120 insertions(+), 19 deletions(-) diff --git a/contracts/common/AHasAutomatons.sol b/contracts/common/AHasAutomatons.sol index 89987b58..d5abc010 100644 --- a/contracts/common/AHasAutomatons.sol +++ b/contracts/common/AHasAutomatons.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.10; import "../lib/LibAddressSet.sol"; import "../lib/LibPaginate.sol"; -import "../common/lib/LibHasAutomatons.sol"; +import "./lib/LibHasAutomatons.sol"; /** * @title The Fast Smart Contract. diff --git a/contracts/common/AHasForwarder.sol b/contracts/common/AHasForwarder.sol index 39abbe1c..6c43b04a 100644 --- a/contracts/common/AHasForwarder.sol +++ b/contracts/common/AHasForwarder.sol @@ -2,20 +2,55 @@ pragma solidity 0.8.10; import "./lib/LibHasForwarder.sol"; + import "@opengsn/contracts/src/ERC2771Recipient.sol"; /** - * @title The Forwarder Smart Contract. - * @notice The HasForwarder abstract contract is in charge of... + * @title The Forwarder behaviour abstract contract. + * @notice The AHasForwarder abstract contract is in charge of adding the Forwarder + * functionality to any contract inheriting from it. */ abstract contract AHasForwarder is ERC2771Recipient { + /// Errors. + + /** + * @notice Happens when a function is called by a non forwarder manager. + * @param who is the address that called the function. + */ + error RequiresForwarderManager(address who); + + /// Events. + + /** + * @notice Emited when a forwarder is set on an implementing contract. + * @param forwarderAddress is the address of the trusted forwarder. + */ + event ForwarderChanged(address forwarderAddress); + /** * @notice ERC2771Recipient implementation. - * TODO: FIX MODIFIER + * @param _forwarderAddress the forwarder address. */ - function setTrustedForwarder(address _forwarderAddress) external { + function setTrustedForwarder(address _forwarderAddress) external onlyForwarderManager { LibHasForwarder.Data storage ds = LibHasForwarder.data(); ds.forwarderAddress = _forwarderAddress; _setTrustedForwarder(_forwarderAddress); + + emit ForwarderChanged(_forwarderAddress); + } + + /** + * @notice Checks whether the given address is a forwarder manager or not. + * @dev Must be implemented by the inheriting contract. + * @param who is the address to test. + */ + function isValidForwarderManager(address who) internal view virtual returns (bool); + + /// Modifiers. + + /// @notice Ensures that a method can only be called by the forwarder manager. + modifier onlyForwarderManager() virtual { + if (!isValidForwarderManager(msg.sender)) revert RequiresForwarderManager(msg.sender); + _; } } diff --git a/contracts/common/AHasGovernors.sol b/contracts/common/AHasGovernors.sol index 8de2c0e1..1bcd00cf 100644 --- a/contracts/common/AHasGovernors.sol +++ b/contracts/common/AHasGovernors.sol @@ -3,8 +3,8 @@ pragma solidity 0.8.10; import "../lib/LibAddressSet.sol"; import "../lib/LibPaginate.sol"; -import "../common/lib/LibHasGovernors.sol"; import "../interfaces/ICustomErrors.sol"; +import "./lib/LibHasGovernors.sol"; /** * @title The Fast Smart Contract. diff --git a/contracts/common/AHasMembers.sol b/contracts/common/AHasMembers.sol index 5967df59..c870ddc3 100644 --- a/contracts/common/AHasMembers.sol +++ b/contracts/common/AHasMembers.sol @@ -3,8 +3,8 @@ pragma solidity 0.8.10; import "../lib/LibAddressSet.sol"; import "../lib/LibPaginate.sol"; -import "../common/lib/LibHasMembers.sol"; import "../interfaces/ICustomErrors.sol"; +import "./lib/LibHasMembers.sol"; /** * @title The Fast Smart Contract. @@ -13,6 +13,8 @@ import "../interfaces/ICustomErrors.sol"; abstract contract AHasMembers { using LibAddressSet for LibAddressSet.Data; + function _msgSender() internal view virtual returns (address); + /// Errors. /// @notice Happens when a function is called by an address that is not a members manager. @@ -94,7 +96,7 @@ abstract contract AHasMembers { * @notice Adds a member to the list of known members. * @param who is the address to be added. */ - function addMember(address who) external onlyMemberManager(msg.sender) onlyValidMember(who) { + function addMember(address who) external onlyMemberManager(_msgSender()) onlyValidMember(who) { // Add the member. LibHasMembers.data().memberSet.add(who, false); // Notify via callback. @@ -109,7 +111,7 @@ abstract contract AHasMembers { * @notice Requires that the caller is a member of this Issuer. * @notice Emits a `AHasMembers.MemberRemoved` event. */ - function removeMember(address member) external onlyMemberManager(msg.sender) { + function removeMember(address member) external onlyMemberManager(_msgSender()) { // Notify via callback. onMemberRemoved(member); // Remove member. diff --git a/contracts/fast/FastAccessFacet.sol b/contracts/fast/FastAccessFacet.sol index 6efc9cd1..33242d2a 100644 --- a/contracts/fast/FastAccessFacet.sol +++ b/contracts/fast/FastAccessFacet.sol @@ -19,7 +19,7 @@ import "./FastAutomatonsFacet.sol"; * @notice The FAST Access facet is the source of truth when it comes to * permissioning and ACLs within a given FAST. */ -contract FastAccessFacet is AFastFacet, AHasGovernors, AHasMembers { +contract FastAccessFacet is AFastFacet, AHasGovernors, AHasMembers, AHasForwarder { using LibAddressSet for LibAddressSet.Data; /// Structs. @@ -61,8 +61,21 @@ contract FastAccessFacet is AFastFacet, AHasGovernors, AHasMembers { FastFrontendFacet(address(this)).emitDetailsChanged(); } + /// AHasForwarder implementation. + + // For now the forwarder manager is an issuer. + function isValidForwarderManager(address who) internal view override(AHasForwarder) returns (bool) { + return AHasMembers(this).isMember(who); + } + /// AHasMembers implementation. + // Horrible override... + // Delegates to ERC2771Recipient. + function _msgSender() internal view override(AHasMembers, ERC2771Recipient) returns (address) { + return super._msgSender(); + } + function isMembersManager(address who) internal view override(AHasMembers) returns (bool) { return // The current contract should be able to manage its own members. diff --git a/contracts/interfaces/ICustomErrors.sol b/contracts/interfaces/ICustomErrors.sol index ff90222c..7ff1dc54 100644 --- a/contracts/interfaces/ICustomErrors.sol +++ b/contracts/interfaces/ICustomErrors.sol @@ -23,6 +23,7 @@ interface ICustomErrors { error RequiresFastGovernorship(address who); error RequiresFastMemberCaller(); error RequiresFastMembership(address who); + error RequiresForwarderManager(address who); error RequiresGovernorsManager(address who); error RequiresIssuerMemberCaller(); error RequiresIssuerMemberOrIssuerCaller(); diff --git a/contracts/issuer/IssuerAccessFacet.sol b/contracts/issuer/IssuerAccessFacet.sol index 44745f62..3a4d56ce 100644 --- a/contracts/issuer/IssuerAccessFacet.sol +++ b/contracts/issuer/IssuerAccessFacet.sol @@ -5,6 +5,7 @@ import "../lib/LibAddressSet.sol"; import "../lib/LibPaginate.sol"; import "../lib/LibHelpers.sol"; import "../common/AHasMembers.sol"; +import "../common/AHasForwarder.sol"; import "../interfaces/ICustomErrors.sol"; import "../fast/FastTopFacet.sol"; import "../fast/FastTokenFacet.sol"; @@ -13,11 +14,24 @@ import "./lib/LibIssuerAccess.sol"; import "./lib/IIssuerEvents.sol"; import "../issuer/IssuerTopFacet.sol"; -contract IssuerAccessFacet is AIssuerFacet, AHasMembers { +contract IssuerAccessFacet is AIssuerFacet, AHasMembers, AHasForwarder { using LibAddressSet for LibAddressSet.Data; + /// AHasForwarder implementation. + + // For now the forwarder manager is an issuer. + function isValidForwarderManager(address who) internal view override(AHasForwarder) returns (bool) { + return AHasMembers(this).isMember(who); + } + /// AHasMembers implementation. + // Horrible override... + // Delegates to ERC2771Recipient. + function _msgSender() internal view override(AHasMembers, ERC2771Recipient) returns (address) { + return super._msgSender(); + } + function isMembersManager(address who) internal view override(AHasMembers) returns (bool) { return AHasMembers(this).isMember(who); } diff --git a/contracts/marketplace/MarketplaceAccessFacet.sol b/contracts/marketplace/MarketplaceAccessFacet.sol index f5983cb5..288d3e6c 100644 --- a/contracts/marketplace/MarketplaceAccessFacet.sol +++ b/contracts/marketplace/MarketplaceAccessFacet.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.10; import "../lib/LibAddressSet.sol"; import "../lib/LibPaginate.sol"; import "../common/AHasMembers.sol"; +import "../common/AHasForwarder.sol"; import "../issuer/IssuerTopFacet.sol"; import "../interfaces/ICustomErrors.sol"; import "../interfaces/IHasActiveMembers.sol"; @@ -15,11 +16,24 @@ import "./MarketplaceAutomatonsFacet.sol"; * @title The Marketplace Smart Contract. * @notice The Marketplace Access facet is in charge of keeping track of marketplace members. */ -contract MarketplaceAccessFacet is AMarketplaceFacet, AHasMembers, IHasActiveMembers { +contract MarketplaceAccessFacet is AMarketplaceFacet, AHasMembers, AHasForwarder, IHasActiveMembers { using LibAddressSet for LibAddressSet.Data; + /// AHasForwarder implementation. + + // For now the forwarder manager is an issuer. + function isValidForwarderManager(address who) internal view override(AHasForwarder) returns (bool) { + return _isIssuerMember(who); + } + /// AHasMembers implementation. + // Horrible override... + // Delegates to ERC2771Recipient. + function _msgSender() internal view override(AHasMembers, ERC2771Recipient) returns (address) { + return super._msgSender(); + } + function isMembersManager(address who) internal view override(AHasMembers) returns (bool) { return _isIssuerMember(who) || AHasAutomatons(address(this)).automatonCan(who, MARKETPLACE_PRIVILEGE_MANAGE_MEMBERS); } @@ -56,12 +70,12 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, AHasMembers, IHasActiveMem */ function memberAddedToFast(address member) external { // Verify that the given address is in fact a registered FAST contract. - if (!IssuerTopFacet(LibMarketplace.data().issuer).isFastRegistered(msg.sender)) { + if (!IssuerTopFacet(LibMarketplace.data().issuer).isFastRegistered(_msgSender())) { revert ICustomErrors.RequiresFastContractCaller(); } // Keep track of the member's FAST membership. // TODO: We don't throw until we've fixed the `marketplace.fastMemberships`. - LibMarketplaceAccess.data().fastMemberships[member].add(msg.sender, true); + LibMarketplaceAccess.data().fastMemberships[member].add(_msgSender(), true); } /** @@ -69,12 +83,12 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, AHasMembers, IHasActiveMem * @param member The member for which a FAST membership has been removed. */ function memberRemovedFromFast(address member) external { - if (!IssuerTopFacet(LibMarketplace.data().issuer).isFastRegistered(msg.sender)) { + if (!IssuerTopFacet(LibMarketplace.data().issuer).isFastRegistered(_msgSender())) { revert ICustomErrors.RequiresFastContractCaller(); } // Remove the tracked membership. // TODO: We don't throw until we've fixed the `marketplace.fastMemberships`. - LibMarketplaceAccess.data().fastMemberships[member].remove(msg.sender, true); + LibMarketplaceAccess.data().fastMemberships[member].remove(_msgSender(), true); } /// IHasActiveMembers implementation. diff --git a/test/marketplace/MarketplaceAccessFacet.test.ts b/test/marketplace/MarketplaceAccessFacet.test.ts index 0287bff0..8c3c53e6 100644 --- a/test/marketplace/MarketplaceAccessFacet.test.ts +++ b/test/marketplace/MarketplaceAccessFacet.test.ts @@ -24,13 +24,16 @@ describe("MarketplaceAccessFacet", () => { alice: SignerWithAddress, bob: SignerWithAddress, rob: SignerWithAddress, - john: SignerWithAddress; + john: SignerWithAddress, + trustedForwarder: SignerWithAddress; let issuer: FakeContract, fast: FakeContract, marketplace: Marketplace, access: MarketplaceAccessFacet, - issuerMemberAccess: MarketplaceAccessFacet; + issuerMemberAccess: MarketplaceAccessFacet, + trustedForwarderAccess: MarketplaceAccessFacet, + deployerAccess: MarketplaceAccessFacet; const marketplaceDeployFixture = deployments.createFixture( marketplaceFixtureFunc @@ -50,7 +53,7 @@ describe("MarketplaceAccessFacet", () => { before(async () => { // Keep track of a few signers. - [deployer, issuerMember, alice, bob, rob, john] = await ethers.getSigners(); + [deployer, issuerMember, alice, bob, rob, john, trustedForwarder] = await ethers.getSigners(); // Mock Issuer and Fast contracts. issuer = await smock.fake("Issuer"); fast = await smock.fake("Fast"); @@ -68,6 +71,8 @@ describe("MarketplaceAccessFacet", () => { marketplace.address ); issuerMemberAccess = access.connect(issuerMember); + deployerAccess = access.connect(deployer); + trustedForwarderAccess = access.connect(trustedForwarder); }, }, initWith: { @@ -146,6 +151,23 @@ describe("MarketplaceAccessFacet", () => { await expect(subject).to.have.revertedWith(`RequiresMembersManager`); }); + it("accepts a trusted forwarder as the _msgSender()", async () => { + // Set the trusted forwarder. + await deployerAccess.setTrustedForwarder(trustedForwarder.address); + + // Now add member... + const encodedFunctionCall = await trustedForwarderAccess.interface.encodeFunctionData("addMember", [alice.address]); + const data = `${encodedFunctionCall}${issuerMember.address.slice(2)}`; + + // Override the transaction data. + // Should now appear from the Issuer. + await trustedForwarder.sendTransaction({ data: data, to: access.address }); + + // Check that Alice is a member. + const subject = await marketplace.isMember(alice.address); + expect(subject).to.eq(true); + }); + it("delegates to the Issuer for permission", async () => { await issuerMemberAccess.addMember(alice.address); expect(issuer.isMember).to.be.calledOnceWith(issuerMember.address);