From df2329f11915a2b07facf4dd706663798fa6837c Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Wed, 4 Sep 2024 11:33:30 +0100 Subject: [PATCH] More proxy-colony permission tests (and others) --- .circleci/config.yml | 5 ++ contracts/bridging/ProxyColony.sol | 8 ++- .../colony/ColonyArbitraryTransaction.sol | 1 + .../colonyNetwork/ColonyNetworkSkills.sol | 1 - .../colonyNetwork/ColonyNetworkStorage.sol | 13 +--- .../colony-network-extensions.js | 7 ++ test/contracts-network/colony-network.js | 12 +--- test/contracts-network/metatx-token.js | 6 ++ .../reputation-basic-functionality.js | 52 +++++++++++++- test/cross-chain/cross-chain.js | 71 ++++++++++++++++++- test/misc/chainid.js | 29 ++++++-- 11 files changed, 169 insertions(+), 36 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f152199f8f..76e398d03f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -318,6 +318,11 @@ jobs: command: CHAIN_ID=100 pnpm run test:contracts:chainid:coverage environment: NODE_OPTIONS: --max-old-space-size=6144 + - run: + name: "Running chainid tests for Mainnet" + command: CHAIN_ID=1 pnpm run test:contracts:chainid:coverage + environment: + NODE_OPTIONS: --max-old-space-size=6144 - persist_to_workspace: root: ./ paths: diff --git a/contracts/bridging/ProxyColony.sol b/contracts/bridging/ProxyColony.sol index 01351d59e3..4cb0b93851 100644 --- a/contracts/bridging/ProxyColony.sol +++ b/contracts/bridging/ProxyColony.sol @@ -96,9 +96,13 @@ contract ProxyColony is DSAuth, Multicall, CallWithGuards { require(_targets[i] != bridgeAddress, "colony-cannot-target-bridge"); require(_targets[i] != owner, "colony-cannot-target-network"); - (bool success, ) = callWithGuards(_targets[i], _payloads[i]); + (bool success, bytes memory returndata) = callWithGuards(_targets[i], _payloads[i]); - require(success, "colony-arbitrary-transaction-failed"); + // Note that this is not a require because returndata might not be a string, and if we try + // to decode it we'll get a revert. + if (!success) { + revert(abi.decode(returndata, (string))); + } } } } diff --git a/contracts/colony/ColonyArbitraryTransaction.sol b/contracts/colony/ColonyArbitraryTransaction.sol index 8938650221..00dea9eb5c 100644 --- a/contracts/colony/ColonyArbitraryTransaction.sol +++ b/contracts/colony/ColonyArbitraryTransaction.sol @@ -61,6 +61,7 @@ contract ColonyArbitraryTransaction is ColonyStorage { bytes memory payload = abi.encodeWithSignature("multicall(bytes[])", _actions); IColonyNetwork(colonyNetworkAddress).bridgeMessageToNetwork(_chainId, payload); + return true; } function makeArbitraryTransactions( diff --git a/contracts/colonyNetwork/ColonyNetworkSkills.sol b/contracts/colonyNetwork/ColonyNetworkSkills.sol index adcf33232e..4e4b101bd5 100644 --- a/contracts/colonyNetwork/ColonyNetworkSkills.sol +++ b/contracts/colonyNetwork/ColonyNetworkSkills.sol @@ -22,7 +22,6 @@ import "./../reputationMiningCycle/IReputationMiningCycle.sol"; import "./../common/Multicall.sol"; import "./ColonyNetworkStorage.sol"; import { IColonyBridge } from "./../bridging/IColonyBridge.sol"; -import { CallWithGuards } from "../common/CallWithGuards.sol"; contract ColonyNetworkSkills is ColonyNetworkStorage, Multicall { // Skills diff --git a/contracts/colonyNetwork/ColonyNetworkStorage.sol b/contracts/colonyNetwork/ColonyNetworkStorage.sol index e2324bb9a3..861c0005d9 100644 --- a/contracts/colonyNetwork/ColonyNetworkStorage.sol +++ b/contracts/colonyNetwork/ColonyNetworkStorage.sol @@ -24,18 +24,11 @@ import { CommonStorage } from "./../common/CommonStorage.sol"; import { MultiChain } from "./../common/MultiChain.sol"; import { ERC20Extended } from "./../common/ERC20Extended.sol"; import { ColonyNetworkDataTypes } from "./ColonyNetworkDataTypes.sol"; -import { CallWithGuards } from "../common/CallWithGuards.sol"; // ignore-file-swc-131 // ignore-file-swc-108 -contract ColonyNetworkStorage is - ColonyNetworkDataTypes, - DSMath, - CommonStorage, - MultiChain, - CallWithGuards -{ +contract ColonyNetworkStorage is ColonyNetworkDataTypes, DSMath, CommonStorage, MultiChain { // Number of colonies in the network uint256 colonyCount; // Storage slot 6 // uint256 version number of the latest deployed Colony contract, used in creating new colonies @@ -193,10 +186,6 @@ contract ColonyNetworkStorage is return _skillId >> 128; } - function isMiningChain() internal view returns (bool) { - return block.chainid == getMiningChainId(); - } - function getMiningChainId() public view returns (uint256) { return reputationMiningChainId; } diff --git a/test/contracts-network/colony-network-extensions.js b/test/contracts-network/colony-network-extensions.js index 2f0de76756..73eba4ff02 100644 --- a/test/contracts-network/colony-network-extensions.js +++ b/test/contracts-network/colony-network-extensions.js @@ -305,6 +305,13 @@ contract("Colony Network Extensions", (accounts) => { }); describe("using extensions", () => { + it("extension-managing functions on Network cannot be called by non-colony addresses", async () => { + await checkErrorRevert(colonyNetwork.installExtension(TEST_EXTENSION, 1, { from: ROOT }), "colony-caller-must-be-colony"); + await checkErrorRevert(colonyNetwork.upgradeExtension(TEST_EXTENSION, 2, { from: ROOT }), "colony-caller-must-be-colony"); + await checkErrorRevert(colonyNetwork.deprecateExtension(TEST_EXTENSION, true, { from: ROOT }), "colony-caller-must-be-colony"); + await checkErrorRevert(colonyNetwork.uninstallExtension(TEST_EXTENSION, { from: ROOT }), "colony-caller-must-be-colony"); + }); + it("allows network-managed extensions to lock and unlock tokens", async () => { const tokenLockingAddress = await colonyNetwork.getTokenLocking(); const tokenLocking = await ITokenLocking.at(tokenLockingAddress); diff --git a/test/contracts-network/colony-network.js b/test/contracts-network/colony-network.js index 6df1a16654..e2518da82b 100755 --- a/test/contracts-network/colony-network.js +++ b/test/contracts-network/colony-network.js @@ -26,7 +26,7 @@ const { setStorageSlot, } = require("../../helpers/test-helper"); -const { CURR_VERSION, MIN_STAKE, IPFS_HASH, ADDRESS_ZERO, WAD } = require("../../helpers/constants"); +const { CURR_VERSION, IPFS_HASH, ADDRESS_ZERO, WAD } = require("../../helpers/constants"); const { setupENSRegistrar } = require("../../helpers/upgradable-contracts"); const { expect } = chai; @@ -203,16 +203,6 @@ contract("Colony Network", (accounts) => { await checkErrorRevert(colonyNetwork.startNextCycle(), "colony-reputation-mining-clny-token-invalid-address"); }); - - it('should not allow "punishStakers" to be called from an account that is not the mining cycle', async () => { - const chainId = await getChainId(); - await metaColony.initialiseReputationMining(chainId, ethers.constants.HashZero, 0); - - await checkErrorRevert( - colonyNetwork.punishStakers([accounts[0], accounts[1]], MIN_STAKE), - "colony-reputation-mining-sender-not-active-reputation-cycle", - ); - }); }); describe("when creating new colonies at a specific version", () => { diff --git a/test/contracts-network/metatx-token.js b/test/contracts-network/metatx-token.js index 88139cbf0b..320efaacb4 100644 --- a/test/contracts-network/metatx-token.js +++ b/test/contracts-network/metatx-token.js @@ -289,6 +289,12 @@ contract("MetaTxToken", (accounts) => { const locked = await metaTxToken.locked(); expect(locked).to.be.true; }); + + it("only owner can call setOwner", async () => { + await checkErrorRevert(metaTxToken.setOwner(USER1, { from: USER1 }), "ds-auth-unauthorized"); + await metaTxToken.setOwner(USER1, { from: USER0 }); + await metaTxToken.setOwner(USER0, { from: USER1 }); + }); }); describe("when working with ether transfers", () => { diff --git a/test/contracts-network/reputation-basic-functionality.js b/test/contracts-network/reputation-basic-functionality.js index bed0d2bd07..39b8d77edc 100644 --- a/test/contracts-network/reputation-basic-functionality.js +++ b/test/contracts-network/reputation-basic-functionality.js @@ -6,7 +6,7 @@ const bnChai = require("bn-chai"); const { ethers } = require("ethers"); const { giveUserCLNYTokens, giveUserCLNYTokensAndStake } = require("../../helpers/test-data-generator"); -const { MIN_STAKE, MINING_CYCLE_DURATION, DECAY_RATE, CHALLENGE_RESPONSE_WINDOW_DURATION } = require("../../helpers/constants"); +const { MIN_STAKE, MINING_CYCLE_DURATION, DECAY_RATE, CHALLENGE_RESPONSE_WINDOW_DURATION, ADDRESS_ZERO } = require("../../helpers/constants"); const { forwardTime, checkErrorRevert, getActiveRepCycle, advanceMiningCycleNoContest, getBlockTime } = require("../../helpers/test-helper"); const { expect } = chai; @@ -213,9 +213,23 @@ contract("Reputation mining - basic functionality", (accounts) => { await checkErrorRevert(repCycle.resetWindow(), "colony-reputation-mining-sender-not-network"); }); - it('should not allow "setReputationRootHash" to be called from an account that is not a ReputationMiningCycle', async () => { + it("should not allow reputation-mining functions to be called from an account that is not an active ReputationMiningCycle", async () => { + const inactiveRepCycleAddr = await colonyNetwork.getReputationMiningCycle(false); + + await checkErrorRevert( + colonyNetwork.setReputationRootHash("0x000001", 10, [accounts[0], accounts[1]], { from: inactiveRepCycleAddr }), + "colony-reputation-mining-sender-not-active-reputation-cycle", + ); + await checkErrorRevert( + colonyNetwork.punishStakers([accounts[0], accounts[1]], MIN_STAKE, { from: inactiveRepCycleAddr }), + "colony-reputation-mining-sender-not-active-reputation-cycle", + ); + await checkErrorRevert( + colonyNetwork.reward(accounts[0], MIN_STAKE, { from: inactiveRepCycleAddr }), + "colony-reputation-mining-sender-not-active-reputation-cycle", + ); await checkErrorRevert( - colonyNetwork.setReputationRootHash("0x000001", 10, [accounts[0], accounts[1]]), + colonyNetwork.burnUnneededRewards(0, { from: inactiveRepCycleAddr }), "colony-reputation-mining-sender-not-active-reputation-cycle", ); }); @@ -258,6 +272,38 @@ contract("Reputation mining - basic functionality", (accounts) => { await checkErrorRevert(inactiveRepCycle.initialise(MINER1, MINER2), "colony-reputation-mining-cycle-already-initialised"); }); + + it("functions that should only be after initialisation cannot be called before", async () => { + const before = await web3.eth.getStorageAt(colonyNetwork.address, 17); + const ethersProvider = new ethers.providers.Web3Provider(web3.currentProvider); + await ethersProvider.send("hardhat_setStorageAt", [ + colonyNetwork.address, + ethers.utils.hexZeroPad(ethers.utils.hexlify(17), 32), + ethers.utils.hexZeroPad(ethers.utils.hexlify(0), 32), + ]); + + await checkErrorRevert(colonyNetwork.startNextCycle(), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.setMiningDelegate(ADDRESS_ZERO, false), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.setReputationRootHash("0x00", 0, [ADDRESS_ZERO]), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.punishStakers([ADDRESS_ZERO], 0), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.reward(ADDRESS_ZERO, 0), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.claimMiningReward(ADDRESS_ZERO), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.unstakeForMining(0), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.burnUnneededRewards(0), "colony-reputation-mining-not-initialised"); + await checkErrorRevert(colonyNetwork.setReputationMiningCycleReward(0), "colony-reputation-mining-not-initialised"); + + // Put in to recovery mode + await colonyNetwork.enterRecoveryMode(); + + await checkErrorRevert( + colonyNetwork.setReplacementReputationUpdateLogEntry(ADDRESS_ZERO, 0, ADDRESS_ZERO, 0, 0, ADDRESS_ZERO, 0, 0), + "colony-reputation-mining-not-initialised", + ); + await colonyNetwork.approveExitRecovery(); + await colonyNetwork.exitRecoveryMode(); + + await ethersProvider.send("hardhat_setStorageAt", [colonyNetwork.address, ethers.utils.hexZeroPad(ethers.utils.hexlify(17), 32), before]); + }); }); describe("when reading reputation mining constant properties", async () => { diff --git a/test/cross-chain/cross-chain.js b/test/cross-chain/cross-chain.js index e626ffc0df..c659f7e42f 100644 --- a/test/cross-chain/cross-chain.js +++ b/test/cross-chain/cross-chain.js @@ -315,6 +315,19 @@ contract("Cross-chain", (accounts) => { await checkErrorRevertEthers(tx.wait(), "colony-caller-must-be-meta-colony"); }); + it("callProxyNetwork can only be called through the metacolony", async () => { + const payload = homeColonyNetwork.interface.encodeFunctionData("setColonyBridgeAddress", [ADDRESS_ZERO]); + let tx = await homeColonyNetwork.createColonyForFrontend(ADDRESS_ZERO, "A", "A", 18, CURR_VERSION, "", ""); + await tx.wait(); + + const colonyCount = await homeColonyNetwork.getColonyCount(); + const colonyAddress = await homeColonyNetwork.getColony(colonyCount); + const fakeMetaColony = new ethers.Contract(colonyAddress, IMetaColony.abi, ethersHomeSigner); + + tx = await fakeMetaColony.callProxyNetwork(foreignChainId, [payload], { gasLimit: 1000000 }); + await checkErrorRevertEthers(tx.wait(), "colony-caller-must-be-meta-colony"); + }); + it("callProxyNetwork can only be called by root permissions on the metacolony", async () => { const payload = remoteColonyNetwork.interface.encodeFunctionData("setColonyBridgeAddress", [ADDRESS_ZERO]); const homeMetacolony2 = new ethers.Contract(homeMetacolony.address, IMetaColony.abi, ethersHomeSigner2); @@ -698,6 +711,50 @@ contract("Cross-chain", (accounts) => { expect(colonyBalance.toHexString()).to.equal(ethers.utils.parseEther("0.7").toHexString()); expect(recipientBalanceAfter.sub(receipientBalanceBefore).toHexString()).to.equal(ethers.utils.parseEther("0.3").toHexString()); }); + + it("a bookkeeping error will mean that tokens can no longer be claimed until tokens are returned", async () => { + const tokenAmount = ethers.utils.parseEther("100"); + + let tx = await foreignToken["mint(address,uint256)"](proxyColony.address, tokenAmount); + await tx.wait(); + + // Claim on the foreign chain + const p = bridgeMonitor.getPromiseForNextBridgedTransaction(); + tx = await proxyColony.claimTokens(foreignToken.address); + await tx.wait(); + await p; + + // Now remove the tokens + const balanceSlot = ethers.utils.keccak256( + ethers.utils.concat([ethers.utils.hexZeroPad(proxyColony.address, 32), ethers.utils.hexZeroPad(1, 32)]), + ); + + await ethersForeignProvider.send("hardhat_setStorageAt", [ + foreignToken.address, + balanceSlot, + ethers.utils.hexZeroPad(ethers.utils.parseEther("30").toHexString(), 32), + ]); + + tx = await proxyColony.claimTokens(foreignToken.address, { gasLimit: 1000000 }); + await checkErrorRevertEthers(tx.wait(), "colony-shell-token-bookkeeping-error"); + + // Now return the tokens + await ethersForeignProvider.send("hardhat_setStorageAt", [ + foreignToken.address, + balanceSlot, + ethers.utils.hexZeroPad(ethers.utils.parseEther("100").toHexString(), 32), + ]); + + // Mint some more tokens + tx = await foreignToken["mint(address,uint256)"](proxyColony.address, ethers.utils.parseEther("100")); + await tx.wait(); + + // Can now claim + const p2 = bridgeMonitor.getPromiseForNextBridgedTransaction(); + tx = await proxyColony.claimTokens(foreignToken.address); + await tx.wait(); + await p2; + }); }); describe("making arbitrary transactions on another chain", async () => { @@ -741,6 +798,18 @@ contract("Cross-chain", (accounts) => { expect(balanceAfter.sub(balanceBefore).toHexString()).to.equal(ethers.utils.parseEther("100").toHexString()); }); + it("arbitrary transactions on the foreign chain must go to contracts", async () => { + const p = bridgeMonitor.getPromiseForNextBridgedTransaction(); + + const payload = foreignToken.interface.encodeFunctionData("mint(address,uint256)", [proxyColony.address, ethers.utils.parseEther("100")]); + + const tx = await colony.makeProxyArbitraryTransactions(foreignChainId, [accounts[0]], [payload]); + await tx.wait(); + await p; + + await checkErrorRevertEthers(p, "require-execute-call-target-not-contract"); + }); + it("can make multiple arbitrary transactions on the foreign chain in one go", async () => { const shellBalanceBefore = await foreignToken.balanceOf(proxyColony.address); const colonyBalanceBefore = await colony.getFundingPotProxyBalance(1, foreignChainId, foreignToken.address); @@ -790,7 +859,7 @@ contract("Cross-chain", (accounts) => { const tx4 = await colony.makeProxyArbitraryTransactions(foreignChainId, [foreignToken.address], ["0x00000000"]); await tx4.wait(); - await checkErrorRevertEthers(p, "colony-arbitrary-transaction-failed"); + await checkErrorRevertEthers(p, "require-execute-call-reverted-with-no-error"); }); }); diff --git a/test/misc/chainid.js b/test/misc/chainid.js index c4560fd169..a9da688d20 100644 --- a/test/misc/chainid.js +++ b/test/misc/chainid.js @@ -99,13 +99,23 @@ contract("Contract Storage", (accounts) => { it("should handle tokens appropriately if auction is initialised for the CLNY token", async () => { await giveUserCLNYTokens(colonyNetwork, colonyNetwork.address, WAD); const supplyBefore = await clnyToken.totalSupply(); + const balanceBefore = await clnyToken.balanceOf(colonyNetwork.address); const tx = await colonyNetwork.startTokenAuction(clnyToken.address); const supplyAfter = await clnyToken.totalSupply(); + const balanceAfter = await clnyToken.balanceOf(colonyNetwork.address); - // tokens should be transferred to metacolony - expect(supplyBefore).to.eq.BN(supplyAfter); - await expectEvent(tx, "Transfer(address indexed,address indexed,uint256)", [colonyNetwork.address, metaColony.address, WAD]); + if (await isMainnet()) { + // tokens should be burned. + expect(supplyBefore.sub(supplyAfter)).to.eq.BN(balanceBefore); + await expectEvent(tx, "Burn(address indexed,uint256)", [colonyNetwork.address, WAD]); + expect(balanceAfter).to.be.zero; + expect(supplyBefore.sub(balanceBefore)).to.eq.BN(supplyAfter); + } else { + // tokens should be transferred to metacolony + expect(supplyBefore).to.eq.BN(supplyAfter); + await expectEvent(tx, "Transfer(address indexed,address indexed,uint256)", [colonyNetwork.address, metaColony.address, WAD]); + } }); it("CLNY raised from auctions is dealt with appropriately", async () => { @@ -125,6 +135,7 @@ contract("Contract Storage", (accounts) => { await clnyToken.approve(tokenAuction.address, clnyNeededForMaxPriceAuctionSellout, { from: accounts[1] }); await tokenAuction.bid(clnyNeededForMaxPriceAuctionSellout, { from: accounts[1] }); + const balanceBefore = await clnyToken.balanceOf(tokenAuction.address); const supplyBefore = await clnyToken.totalSupply(); const receivedTotal = await tokenAuction.receivedTotal(); expect(receivedTotal).to.not.be.zero; @@ -133,9 +144,15 @@ contract("Contract Storage", (accounts) => { const balanceAfter = await clnyToken.balanceOf(tokenAuction.address); expect(balanceAfter).to.be.zero; const supplyAfter = await clnyToken.totalSupply(); - // tokens should be transferred to metacolony - expect(supplyBefore).to.eq.BN(supplyAfter); - await expectEvent(tx, "Transfer(address indexed,address indexed,uint256)", [tokenAuction.address, metaColony.address, receivedTotal]); + if (await isMainnet()) { + // tokens should be burned. + expect(supplyBefore.sub(supplyAfter)).to.eq.BN(balanceBefore); + await expectEvent(tx, "Burn(address indexed,uint256)", [tokenAuction.address, receivedTotal]); + } else { + // tokens should be transferred to metacolony + expect(supplyBefore).to.eq.BN(supplyAfter); + await expectEvent(tx, "Transfer(address indexed,address indexed,uint256)", [tokenAuction.address, metaColony.address, receivedTotal]); + } }); }); });