From 2225b301ff2bbd04861b69e1fd054312dc23326b Mon Sep 17 00:00:00 2001 From: Felipe Mendes Date: Tue, 4 Apr 2023 16:19:06 -0300 Subject: [PATCH] chore: prevent direct calls to implementation contract (#194) * feat: disable initializers and set paused state for implementation contracts * test: add deploy guarantee tests for interaction with implementation contracts * chore: add buildId and implementationAddress to deployStore script * test: reset initialized state on store of deployed contract for foundry tests * test: optmize deployUninitialized function * test: test if is possible to mint in implementation contract --- contracts/contracts/FleekERC721.sol | 12 ++ contracts/scripts/utils/deploy-store.js | 8 ++ .../test/foundry/FleekERC721/Deploy.t.sol | 2 +- .../test/foundry/FleekERC721/TestBase.sol | 9 +- contracts/test/hardhat/scripts/deploy.ts | 109 ++++++++++++++++++ 5 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 contracts/test/hardhat/scripts/deploy.ts diff --git a/contracts/contracts/FleekERC721.sol b/contracts/contracts/FleekERC721.sol index 66024d2..dc7a85a 100644 --- a/contracts/contracts/FleekERC721.sol +++ b/contracts/contracts/FleekERC721.sol @@ -53,6 +53,18 @@ contract FleekERC721 is mapping(uint256 => address) private _tokenVerifier; mapping(uint256 => bool) private _tokenVerified; + /** + * @dev This constructor sets the state of implementation contract to paused + * and disable initializers, not allowing interactions with the implementation + * contracts. + */ + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _setPausable(true); + _pause(); + _disableInitializers(); + } + /** * @dev Initializes the contract by setting a `name` and a `symbol` to the token collection. */ diff --git a/contracts/scripts/utils/deploy-store.js b/contracts/scripts/utils/deploy-store.js index 6b58ec9..c01011c 100644 --- a/contracts/scripts/utils/deploy-store.js +++ b/contracts/scripts/utils/deploy-store.js @@ -1,3 +1,4 @@ +const { getImplementationAddress } = require('@openzeppelin/upgrades-core'); const { writeFile } = require('./file'); const { existsSync } = require('fs'); const path = require('path'); @@ -38,9 +39,16 @@ const deployStore = async (network, contractName, contract) => { const { buildId, solcInput, abi, bytecode, metadata, storageLayout } = await getBuildData(contractName); + const implementationAddress = await getImplementationAddress( + hre.network.provider, + contract.address + ); + const data = { + buildId, timestamp: new Date().toLocaleString('en-US'), address: contract.address, + implementationAddress, transactionHash: contract.deployTransaction.hash, args: contract.deployTransaction.args, gasPrice: contract.deployTransaction.gasPrice.toNumber(), diff --git a/contracts/test/foundry/FleekERC721/Deploy.t.sol b/contracts/test/foundry/FleekERC721/Deploy.t.sol index e292f08..de64aa1 100644 --- a/contracts/test/foundry/FleekERC721/Deploy.t.sol +++ b/contracts/test/foundry/FleekERC721/Deploy.t.sol @@ -23,7 +23,7 @@ contract Test_FleekERC721_Deploy is Test_FleekERC721_Base { } function testFuzz_nameAndSymbol(string memory _name, string memory _symbol) public { - CuT = new FleekERC721(); + CuT = deployUninitialized(); CuT.initialize(_name, _symbol, new uint256[](0)); assertEq(CuT.name(), _name); diff --git a/contracts/test/foundry/FleekERC721/TestBase.sol b/contracts/test/foundry/FleekERC721/TestBase.sol index e629d2a..3fa55aa 100644 --- a/contracts/test/foundry/FleekERC721/TestBase.sol +++ b/contracts/test/foundry/FleekERC721/TestBase.sol @@ -48,8 +48,15 @@ abstract contract Test_FleekERC721_Base is Test, Test_FleekERC721_Assertions { FleekERC721 internal CuT; // Contract Under Test address internal deployer; + function deployUninitialized() internal returns (FleekERC721) { + FleekERC721 _contract = new FleekERC721(); + vm.store(address(_contract), bytes32(0), bytes32(0)); // Overrides `_initialized` and `_initializing` states + return _contract; + } + function baseSetUp() internal { - CuT = new FleekERC721(); + vm.prank(address(CuT)); + CuT = deployUninitialized(); CuT.initialize("Test Contract", "FLKAPS", new uint256[](0)); deployer = address(this); } diff --git a/contracts/test/hardhat/scripts/deploy.ts b/contracts/test/hardhat/scripts/deploy.ts new file mode 100644 index 0000000..74e5dd9 --- /dev/null +++ b/contracts/test/hardhat/scripts/deploy.ts @@ -0,0 +1,109 @@ +import { expect } from 'chai'; +import * as hre from 'hardhat'; +// eslint-disable-next-line @typescript-eslint/ban-ts-comment +// @ts-ignore +import deploy from '../../../scripts/deploy'; +import { getImplementationAddress } from '@openzeppelin/upgrades-core'; +import { Contract } from 'ethers'; +import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'; +import { Errors, TestConstants } from '../contracts/FleekERC721/helpers'; + +const taskArgs = { + newProxyInstance: false, + name: 'FleekNFAs', + symbol: 'FLKNFA', + billing: [], +}; + +const getImplementationContract = async ( + proxyAddress: string +): Promise => { + const implementationAddress = await getImplementationAddress( + hre.network.provider, + proxyAddress + ); + return hre.ethers.getContractAt('FleekERC721', implementationAddress); +}; + +const deployFixture = async () => { + const [owner] = await hre.ethers.getSigners(); + + const proxy = (await deploy(taskArgs, hre)) as Contract; + + const implementation = await getImplementationContract(proxy.address); + + return { proxy, implementation, owner }; +}; + +describe('Deploy', () => { + let fixture: Awaited>; + + // Suppress console.log + const logger = console.log; + before(() => { + console.log = () => undefined; + }); + after(() => { + console.log = logger; + }); + // -------------------- + + beforeEach(async () => { + fixture = await loadFixture(deployFixture); + }); + + it('should deploy the contract', async () => { + const { proxy, implementation } = fixture; + + expect(proxy.address).to.be.a('string'); + expect(implementation.address).to.be.a('string'); + expect(proxy.address).to.be.not.equal(implementation.address); + }); + + it('should have proxy unpaused and implementation paused', async () => { + const { proxy, implementation } = fixture; + + expect(await proxy.isPaused()).to.be.false; + expect(await implementation.isPaused()).to.be.true; + }); + + it('should not allow initialize implementation contract', async () => { + const { implementation } = fixture; + + await expect( + implementation.initialize( + taskArgs.name, + taskArgs.symbol, + taskArgs.billing + ) + ).to.be.revertedWith('Initializable: contract is already initialized'); + }); + + it('should have owner on proxy but not on implementation', async () => { + const { proxy, implementation, owner } = fixture; + + expect(await proxy.hasCollectionRole(0, owner.address)).to.be.true; + expect(await implementation.hasCollectionRole(0, owner.address)).to.be + .false; + }); + + it('should not allow mint on implementation contract', async () => { + const { implementation, owner } = fixture; + + await expect( + implementation.mint( + owner.address, + TestConstants.MintParams.name, + TestConstants.MintParams.description, + TestConstants.MintParams.externalUrl, + TestConstants.MintParams.ens, + TestConstants.MintParams.commitHash, + TestConstants.MintParams.gitRepository, + TestConstants.MintParams.logo, + TestConstants.MintParams.color, + TestConstants.MintParams.accessPointAutoApprovalSettings, + owner.address + ) + ).to.be.revertedWithCustomError(implementation, Errors.ContractIsPaused); + }); +});