From e3381a11db5e43c5fbd5e4911f7e5b7ca61edac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Volpe?= Date: Mon, 8 Jan 2024 20:47:02 +0100 Subject: [PATCH] Proxy.sol foundry migration (#10864) --- packages/protocol/test-sol/common/Proxy.t.sol | 206 +++++++++++++ packages/protocol/test/common/proxy.ts | 279 ------------------ packages/protocol/test/common/recoverFunds.ts | 65 ++++ 3 files changed, 271 insertions(+), 279 deletions(-) create mode 100644 packages/protocol/test-sol/common/Proxy.t.sol delete mode 100644 packages/protocol/test/common/proxy.ts create mode 100644 packages/protocol/test/common/recoverFunds.ts diff --git a/packages/protocol/test-sol/common/Proxy.t.sol b/packages/protocol/test-sol/common/Proxy.t.sol new file mode 100644 index 00000000000..0ea65646d01 --- /dev/null +++ b/packages/protocol/test-sol/common/Proxy.t.sol @@ -0,0 +1,206 @@ +pragma solidity ^0.5.13; + +import "@celo-contracts/common/Proxy.sol"; + +import "celo-foundry/Test.sol"; + +import { Constants } from "@test-sol/constants.sol"; +import { Utils } from "@test-sol/utils.sol"; + +import "@celo-contracts/common/test/GetSetV0.sol"; +import "@celo-contracts/common/test/GetSetV1.sol"; +import "@celo-contracts/common/test/HasInitializer.sol"; +import "@celo-contracts/common/test/MsgSenderCheck.sol"; + +contract ProxyTest is Test, Constants, Utils { + Proxy proxy; + GetSetV0 getSet; + GetSetV0 proxiedGetSet; + + address nonOwner; + event ImplementationSet(address indexed implementation); + event OwnerSet(address indexed owner); + + function setUp() public { + nonOwner = actor("nonOwner"); + proxy = new Proxy(); + getSet = new GetSetV0(); + proxiedGetSet = new GetSetV0(); + } +} + +contract ProxyTest_getOwner is ProxyTest { + function test_ShouldGetTheAddressOfTheOwner() public { + assertEq(proxy._getOwner(), address(this)); + } +} + +contract ProxyTest_setImplementation is ProxyTest { + function test_ShouldAllowTheOwnerToSetAnImplementation() public { + proxy._setImplementation(address(getSet)); + assertEq(proxy._getImplementation(), address(getSet)); + } + + function test_Reverts_NonOwnerSsetAnImplementation() public { + vm.expectRevert("sender was not owner"); + vm.prank(nonOwner); + proxy._setImplementation(address(getSet)); + } + + function test_shouldAllowTheImplementationToBeUpdated() public { + proxy._setImplementation(address(getSet)); + GetSetV1 getSet1 = new GetSetV1(); + proxy._setImplementation(address(getSet1)); + assertEq(proxy._getImplementation(), address(getSet1)); + } + + function test_ShouldNotAffectLogicRelatedStorage() public { + uint256 numberTotest = 42; + proxy._setImplementation(address(getSet)); + proxiedGetSet.set(numberTotest); + GetSetV1 getSet1 = new GetSetV1(); + proxy._setImplementation(address(getSet1)); + + assertEq(proxiedGetSet.get(), numberTotest); + } + + function test_Emits_ImplementationSet() public { + vm.expectEmit(true, true, true, true); + emit ImplementationSet(address(getSet)); + proxy._setImplementation(address(getSet)); + } +} + +contract ProxyTest_setAndInitializeImplementation is ProxyTest { + HasInitializer hasInitializer; + HasInitializer proxiedHasInitializer; + + function setUp() public { + super.setUp(); + hasInitializer = new HasInitializer(); + proxiedHasInitializer = HasInitializer(address(proxy)); + } + + function test_ShouldAllowTheOwnerToSetAnImplementation() public { + proxy._setAndInitializeImplementation( + address(hasInitializer), + abi.encodeWithSignature("initialize(uint256)", 42) + ); + assertEq(proxy._getImplementation(), address(hasInitializer)); + } + + function test_Emits_ImplementationSet() public { + vm.expectEmit(true, true, true, true); + emit ImplementationSet(address(hasInitializer)); + proxy._setAndInitializeImplementation( + address(hasInitializer), + abi.encodeWithSignature("initialize(uint256)", 42) + ); + } + + function test_Reverts_WhenCalledANonContractAddress() public { + vm.expectRevert("sender was not owner"); + vm.prank(nonOwner); + proxy._setAndInitializeImplementation( + address(nonOwner), + abi.encodeWithSignature("initialize(uint256)", 42) + ); + } + + function test_Reverts_WehCalleBbyANonOwner() public { + vm.expectRevert("sender was not owner"); + vm.prank(nonOwner); + proxy._setAndInitializeImplementation( + address(hasInitializer), + abi.encodeWithSignature("initialize(uint256)", 42) + ); + } + + function test_Reverts_WhenInitializeAlreadyCalled() public { + proxy._setAndInitializeImplementation( + address(hasInitializer), + abi.encodeWithSignature("initialize(uint256)", 42) + ); + vm.expectRevert("contract already initialized"); + proxiedHasInitializer.initialize(43); + } +} + +contract ProxyTest_transferOwnership is ProxyTest { + address newOwner = actor("newOwner"); + + function test_ShouldAllowTheOwnerToTransferOwnership() public { + proxy._transferOwnership(newOwner); + assertEq(proxy._getOwner(), newOwner); + } + + function test_Reverts_ShouldNotAllowANonOwnerToTransferOwnership() public { + vm.prank(nonOwner); + vm.expectRevert("sender was not owner"); + proxy._transferOwnership(nonOwner); + } + + function test_ShouldEmitOwnerSet() public { + vm.expectEmit(true, true, true, true); + emit OwnerSet(newOwner); + proxy._transferOwnership(newOwner); + } + + function test_ShouldAllowTheNewOwnerToPerformOwneronlyActions_AfterTransferingOwnership() public { + GetSetV1 getSet1 = new GetSetV1(); + proxy._transferOwnership(newOwner); + vm.prank(newOwner); + proxy._setImplementation(address(getSet1)); + } + + function test_Reverts_OldOwnerPerformsOwnerOnlyActions_AfterTransferingOwnership() public { + GetSetV1 getSet1 = new GetSetV1(); + proxy._transferOwnership(newOwner); + vm.expectRevert("sender was not owner"); + proxy._setImplementation(address(getSet1)); + } +} + +contract ProxyTest_fallback is ProxyTest { + uint256 numberTotest = 42; + + function setUp() public { + super.setUp(); + proxy._setImplementation(address(getSet)); + } + + function test_ShouldCallFunctionsFromTheTargetContract() public { + proxiedGetSet.set(numberTotest); + assertEq(proxiedGetSet.get(), proxiedGetSet.get()); + } + + function test_ShouldAccessPublicVariablesFromTheTargetContract() public { + proxiedGetSet.set(numberTotest); + assertEq(proxiedGetSet.x(), numberTotest); + } + + function test_ShouldNotAffectTheStorageOfTheTargetContract() public { + proxiedGetSet.set(numberTotest); + assertEq(getSet.get(), 0); + } + + function test_ShouldPreserveMsgSender() public { + MsgSenderCheck msgSenderCheck = new MsgSenderCheck(); + MsgSenderCheck proxiedMsgSenderCheck = MsgSenderCheck(address(msgSenderCheck)); + proxy._setImplementation(address(msgSenderCheck)); + proxiedMsgSenderCheck.checkMsgSender(address(this)); + } + + function test_ShouldBeAbleToProxyToTheNewContract_AfterChangingTheImplementation() public { + GetSetV1 getSet1 = new GetSetV1(); + GetSetV1 proxiedGetSet1 = GetSetV1(address(proxy)); + proxy._setImplementation(address(getSet1)); + + proxiedGetSet1.set(numberTotest, "DON'T PANIC"); + + (uint256 numberReturn, string memory stringReturn) = proxiedGetSet1.get(); + assertEq(numberReturn, numberTotest); + assertEq(stringReturn, "DON'T PANIC"); + } + +} diff --git a/packages/protocol/test/common/proxy.ts b/packages/protocol/test/common/proxy.ts deleted file mode 100644 index d2e0dc8be4b..00000000000 --- a/packages/protocol/test/common/proxy.ts +++ /dev/null @@ -1,279 +0,0 @@ -import { recoverFunds } from '@celo/protocol/lib/recover-funds' -import { CeloContractName } from '@celo/protocol/lib/registry-utils' -import { - assertTransactionRevertWithReason, - expectBigNumberInRange, -} from '@celo/protocol/lib/test-utils' -import { BigNumber } from 'bignumber.js' -import { - FreezerContract, - GetSetV0Instance, - GetSetV1Instance, - GoldTokenContract, - HasInitializerInstance, - MsgSenderCheckInstance, - ProxyInstance, - RegistryContract, -} from 'types' - -const GetSetV0: Truffle.Contract = artifacts.require('GetSetV0') -const GetSetV1: Truffle.Contract = artifacts.require('GetSetV1') -const HasInitializer: Truffle.Contract = artifacts.require( - './contracts/HasInitializer.sol' -) -const MsgSenderCheck: Truffle.Contract = artifacts.require('MsgSenderCheck') -const Proxy: Truffle.Contract = artifacts.require('Proxy') - -contract('Proxy', (accounts: string[]) => { - let proxy: ProxyInstance - let getSet: GetSetV0Instance - let proxiedGetSet: GetSetV0Instance - - const owner = accounts[0] - - beforeEach(async () => { - proxy = await Proxy.new({ from: owner }) - getSet = await GetSetV0.new({ from: owner }) - proxiedGetSet = await GetSetV0.at(proxy.address) - }) - - describe('#getOwner', () => { - it('gets the address of the owner', async () => { - const res = await proxy._getOwner() - assert.equal(res, owner) - }) - }) - - describe('#setImplementation', () => { - it('should allow the owner to set an implementation', async () => { - await proxy._setImplementation(getSet.address) - const res = await proxy._getImplementation() - assert.equal(res, getSet.address) - }) - - it('should not allow a non-owner to set an implementation', async () => { - await assertTransactionRevertWithReason( - proxy._setImplementation(getSet.address, { from: accounts[1] }), - 'sender was not owner' - ) - }) - - it('should allow the implementation to be updated', async () => { - await proxy._setImplementation(getSet.address) - - const getSet1: GetSetV1Instance = await GetSetV1.new() - - await proxy._setImplementation(getSet1.address) - const res = await proxy._getImplementation() - assert.equal(res, getSet1.address) - }) - - it('should not affect logic-related storage', async () => { - await proxy._setImplementation(getSet.address) - await proxiedGetSet.set(42) - - const getSet1: GetSetV1Instance = await GetSetV1.new() - await proxy._setImplementation(getSet1.address) - - const res = await proxiedGetSet.get() - assert.equal(res.toNumber(), 42) - }) - - it('should emit an event', async () => { - const response = await proxy._setImplementation(getSet.address) - const events = response.logs - assert.equal(events.length, 1) - assert.equal(events[0].event, 'ImplementationSet') - }) - }) - - describe('#setAndInitializeImplementation', () => { - let hasInitializer: HasInitializerInstance - let proxiedHasInitializer: HasInitializerInstance - const initializeData = (x: string | number) => { - // @ts-ignore - const initializeAbi = HasInitializer.abi.find( - (abi: any) => abi.type === 'function' && abi.name === 'initialize' - ) - - return web3.eth.abi.encodeFunctionCall(initializeAbi, [x]) - } - - beforeEach(async () => { - hasInitializer = await HasInitializer.new({ from: owner }) - proxiedHasInitializer = await HasInitializer.at(proxy.address) - }) - - it('should allow the owner to set an implementation', async () => { - await proxy._setAndInitializeImplementation(hasInitializer.address, initializeData(42)) - - const res = await proxy._getImplementation() - assert.equal(res, hasInitializer.address) - }) - - it('should initialize data needed by the implementation', async () => { - await proxy._setAndInitializeImplementation(hasInitializer.address, initializeData(42)) - - const res = await proxiedHasInitializer.x() - assert.equal(res.toNumber(), 42) - }) - - it('should emit an event', async () => { - const response = await proxy._setAndInitializeImplementation( - hasInitializer.address, - initializeData(owner) - ) - const events = response.logs - assert.equal(events.length, 1) - assert.equal(events[0].event, 'ImplementationSet') - }) - - it('should not allow to call a non contract address', async () => - assertTransactionRevertWithReason( - proxy._setAndInitializeImplementation(accounts[1], initializeData(42), { - from: accounts[1], - }), - 'sender was not owner' - )) - - it('should not allow a non-owner to set an implementation', async () => - assertTransactionRevertWithReason( - proxy._setAndInitializeImplementation(hasInitializer.address, initializeData(42), { - from: accounts[1], - }), - 'sender was not owner' - )) - - it('should not allow for a call to `initialize` after initialization', async () => { - await proxy._setAndInitializeImplementation(hasInitializer.address, initializeData(42)) - await assertTransactionRevertWithReason( - proxiedHasInitializer.initialize(43), - 'contract already initialized' - ) - }) - }) - - describe('#transferOwnership', () => { - it('should allow the owner to transfer ownership', async () => { - await proxy._transferOwnership(accounts[2]) - const newOwner = await proxy._getOwner() - assert.equal(newOwner, accounts[2]) - }) - - it('should not allow a non-owner to transfer ownership', async () => { - await assertTransactionRevertWithReason( - proxy._transferOwnership(accounts[2], { from: accounts[1] }), - 'sender was not owner' - ) - }) - - it('should emit an event', async () => { - const response = await proxy._transferOwnership(accounts[2]) - const events = response.logs - assert.equal(events.length, 1) - assert.equal(events[0].event, 'OwnerSet') - }) - - describe('after transferring ownership', () => { - let getSet1: GetSetV1Instance - const newOwner: string = accounts[1] - - beforeEach(async () => { - getSet1 = await GetSetV1.new({ from: owner }) - await proxy._transferOwnership(newOwner) - }) - - it('should allow the new owner to perform ownerOnly actions', async () => { - await proxy._setImplementation(getSet1.address, { from: newOwner }) - }) - - it('should not allow the previous owner to perform onlyOwner actions', async () => { - await assertTransactionRevertWithReason( - proxy._setImplementation(getSet1.address), - 'sender was not owner' - ) - }) - }) - }) - - describe('fallback', () => { - beforeEach(async () => { - await proxy._setImplementation(getSet.address) - }) - - it('should call functions from the target contract', async () => { - await proxiedGetSet.set(42) - const res = await proxiedGetSet.get() - assert.equal(res.toNumber(), 42) - }) - - it('should access public variables from the target contract', async () => { - await proxiedGetSet.set(42) - const res = await proxiedGetSet.x() - assert.equal(res.toNumber(), 42) - }) - - it('should not affect the storage of the target contract', async () => { - await proxiedGetSet.set(42) - const res = await getSet.get() - assert.equal(res.toNumber(), 0) - }) - - it('should preserve msg.sender', async () => { - const msgSenderCheck: MsgSenderCheckInstance = await MsgSenderCheck.new() - const proxiedMsgSenderCheck = await MsgSenderCheck.at(proxy.address) - await proxy._setImplementation(msgSenderCheck.address) - - await proxiedMsgSenderCheck.checkMsgSender(owner) - }) - - describe('after changing the implementation', () => { - let getSet1: GetSetV1Instance - let proxiedGetSet1: GetSetV1Instance - - beforeEach(async () => { - getSet1 = await GetSetV1.new({ from: owner }) - proxiedGetSet1 = await GetSetV1.at(proxy.address) - await proxy._setImplementation(getSet1.address) - }) - - it('should be able to proxy to the new contract', async () => { - await proxiedGetSet1.set(42, "DON'T PANIC") - const res = await proxiedGetSet1.get() - assert.equal(res[0].toNumber(), 42) - assert.equal(res[1], "DON'T PANIC") - }) - }) - }) - - it('recovers funds from an incorrectly intialized implementation', async () => { - const Freezer: FreezerContract = artifacts.require('Freezer') - const GoldToken: GoldTokenContract = artifacts.require('GoldToken') - // @ts-ignore - GoldToken.numberFormat = 'BigNumber' - const Registry: RegistryContract = artifacts.require('Registry') - - const freezer = await Freezer.new(true) - const goldToken = await GoldToken.new(true) - const registry = await Registry.new(true) - await registry.setAddressFor(CeloContractName.Freezer, freezer.address) - await goldToken.initialize(registry.address) - - const amount = new BigNumber(10) - const initialBalance = new BigNumber(await goldToken.balanceOf(owner)) - await goldToken.transfer(proxy.address, amount) - - await proxy._setImplementation(getSet.address) - - const ownerBalance = await goldToken.balanceOf(owner) - - expectBigNumberInRange(ownerBalance, initialBalance.minus(amount)) - const proxyBalance = await web3.eth.getBalance(proxy.address) - assert(proxyBalance === amount.toString()) - - await recoverFunds(proxy.address, owner) - const ownerBalance2 = await goldToken.balanceOf(owner) - assert((await web3.eth.getBalance(proxy.address)) === '0') - expectBigNumberInRange(ownerBalance2, initialBalance) - }) -}) diff --git a/packages/protocol/test/common/recoverFunds.ts b/packages/protocol/test/common/recoverFunds.ts new file mode 100644 index 00000000000..496b8c1477b --- /dev/null +++ b/packages/protocol/test/common/recoverFunds.ts @@ -0,0 +1,65 @@ +// Note: this test is testing the recover-funds script and not a particular smart contract. + +import { recoverFunds } from '@celo/protocol/lib/recover-funds' +import { CeloContractName } from '@celo/protocol/lib/registry-utils' +import { expectBigNumberInRange } from '@celo/protocol/lib/test-utils' +import { BigNumber } from 'bignumber.js' +import { + FreezerContract, + GetSetV0Instance, + GoldTokenContract, + ProxyInstance, + RegistryContract, +} from 'types' + +const GetSetV0: Truffle.Contract = artifacts.require('GetSetV0') +const Proxy: Truffle.Contract = artifacts.require('Proxy') + +contract('Proxy', (accounts: string[]) => { + let proxy: ProxyInstance + let getSet: GetSetV0Instance + + const owner = accounts[0] + + beforeEach(async () => { + proxy = await Proxy.new({ from: owner }) + getSet = await GetSetV0.new({ from: owner }) + }) + + describe('fallback', () => { + beforeEach(async () => { + await proxy._setImplementation(getSet.address) + }) + + it('recovers funds from an incorrectly intialized implementation', async () => { + const Freezer: FreezerContract = artifacts.require('Freezer') + const GoldToken: GoldTokenContract = artifacts.require('GoldToken') + // @ts-ignore + GoldToken.numberFormat = 'BigNumber' + const Registry: RegistryContract = artifacts.require('Registry') + + const freezer = await Freezer.new(true) + const goldToken = await GoldToken.new(true) + const registry = await Registry.new(true) + await registry.setAddressFor(CeloContractName.Freezer, freezer.address) + await goldToken.initialize(registry.address) + + const amount = new BigNumber(10) + const initialBalance = new BigNumber(await goldToken.balanceOf(owner)) + await goldToken.transfer(proxy.address, amount) + + await proxy._setImplementation(getSet.address) + + const ownerBalance = await goldToken.balanceOf(owner) + + expectBigNumberInRange(ownerBalance, initialBalance.minus(amount)) + const proxyBalance = await web3.eth.getBalance(proxy.address) + assert(proxyBalance === amount.toString()) + + await recoverFunds(proxy.address, owner) + const ownerBalance2 = await goldToken.balanceOf(owner) + assert((await web3.eth.getBalance(proxy.address)) === '0') + expectBigNumberInRange(ownerBalance2, initialBalance) + }) + }) +})