From d3d9177f4d9ea17c007e8117c2e7b2a0b185ef9a Mon Sep 17 00:00:00 2001 From: Mauro Piazza Date: Tue, 6 Aug 2024 18:34:08 +0200 Subject: [PATCH] chore(tests): add hardhat unit tests for XERC20 --- solidity/src/ptoken-v2/PTokenV2.sol | 3 - solidity/src/ptoken-v2/PTokenV2NoGSN.sol | 3 - solidity/test/hardhat/PTokenV2.test.js | 282 ++++++++++++++--------- 3 files changed, 174 insertions(+), 114 deletions(-) diff --git a/solidity/src/ptoken-v2/PTokenV2.sol b/solidity/src/ptoken-v2/PTokenV2.sol index 8d9d242a..489a9d27 100644 --- a/solidity/src/ptoken-v2/PTokenV2.sol +++ b/solidity/src/ptoken-v2/PTokenV2.sol @@ -49,9 +49,6 @@ contract PTokenV2 is function setFeesManager(address newAddress) external override { address msgSender = _msgSender(); - if (feesManager == address(0) && msgSender != owner()) - // First time - revert("OnlyOwner"); if (feesManager != address(0) && msgSender != feesManager) revert("OnlyFeesManager"); diff --git a/solidity/src/ptoken-v2/PTokenV2NoGSN.sol b/solidity/src/ptoken-v2/PTokenV2NoGSN.sol index c78069b0..ae1e08eb 100644 --- a/solidity/src/ptoken-v2/PTokenV2NoGSN.sol +++ b/solidity/src/ptoken-v2/PTokenV2NoGSN.sol @@ -86,9 +86,6 @@ contract PTokenV2NoGSN is } function setFeesManager(address newAddress) public override { - if (feesManager == address(0) && msg.sender != owner()) - // First time - revert("OnlyOwner"); if (feesManager != address(0) && msg.sender != feesManager) revert("OnlyFeesManager"); diff --git a/solidity/test/hardhat/PTokenV2.test.js b/solidity/test/hardhat/PTokenV2.test.js index d3ce3dd3..b80f5bd5 100644 --- a/solidity/test/hardhat/PTokenV2.test.js +++ b/solidity/test/hardhat/PTokenV2.test.js @@ -1,4 +1,5 @@ import helpers, { loadFixture } from '@nomicfoundation/hardhat-network-helpers' +import assert from 'assert' import { expect } from 'chai' import { ZeroAddress } from 'ethers/constants' import hre from 'hardhat' @@ -13,50 +14,56 @@ import { validateUpgrade } from './utils/validate-upgrade.cjs' const ERC1820 = '0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24' const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) -;['', 'NoGSN'].map(_useGSN => { - describe(`PTokenV2${_useGSN}`, () => { - describe('Storage Layout invariance checks', () => { - const name = 'pToken A' - const symbol = 'pTKN A' - const originChainId = '0x10000000' +;['XERC20', 'PTokenV2', 'PTokenV2NoGSN'].map(_tokenKind => { + describe(`${_tokenKind}`, () => { + const _useGSN = _tokenKind.includes('NoGSN') ? 'NoGSN' : '' - it('Should not detect any storage violation', async () => { - // Set the registry - await deployERC1820() + if (_tokenKind.includes('PToken')) { + describe('Storage Layout invariance checks', () => { + const name = 'pToken A' + const symbol = 'pTKN A' + const originChainId = '0x10000000' - const [_, admin] = await hre.ethers.getSigners() - const pToken = await deployProxy(hre, `PToken${_useGSN}`, [ - name, - symbol, - admin.address, - originChainId, - ]) + it('Should not detect any storage violation', async () => { + // Set the registry + await deployERC1820() - expect(await validateUpgrade(hre, `PTokenV2${_useGSN}`, pToken.target)) - }) + const [_, admin] = await hre.ethers.getSigners() + const pToken = await deployProxy(hre, `PToken${_useGSN}`, [ + name, + symbol, + admin.address, + originChainId, + ]) - it('Should not be possible to upgrade from GSN to non-GSN and viceversa', async () => { - // Set the registry - await deployERC1820() + expect( + await validateUpgrade(hre, `PTokenV2${_useGSN}`, pToken.target), + ) + }) + + it('Should not be possible to upgrade from GSN to non-GSN and viceversa', async () => { + // Set the registry + await deployERC1820() + + const [_, admin] = await hre.ethers.getSigners() + const pToken = await deployProxy(hre, `PToken${_useGSN}`, [ + name, + symbol, + admin.address, + originChainId, + ]) + + const notUseGSN = _useGSN === '' ? 'NoGSN' : '' - const [_, admin] = await hre.ethers.getSigners() - const pToken = await deployProxy(hre, `PToken${_useGSN}`, [ - name, - symbol, - admin.address, - originChainId, - ]) - - _useGSN = _useGSN === '' ? 'NoGSN' : '' - - try { - await validateUpgrade(hre, `PTokenV2${_useGSN}`, pToken.target), - assert.fail('Should never reach here') - } catch (e) { - expect(e.message).to.include('New storage layout is incompatible') - } + try { + await validateUpgrade(hre, `PTokenV2${notUseGSN}`, pToken.target), + assert.fail('Should never reach here') + } catch (e) { + expect(e.message).to.include('New storage layout is incompatible') + } + }) }) - }) + } describe('Tests units', () => { const setup = async () => { @@ -68,55 +75,60 @@ const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) await deployERC1820() - const pToken = await deployProxy(hre, `PToken${_useGSN}`, [ - name, - symbol, - owner.address, - originChainId, - ]) + const pToken = + _tokenKind == 'XERC20' + ? await deploy(hre, 'XERC20', [name, symbol, ZeroAddress]) + : await deployProxy(hre, `PToken${_useGSN}`, [ + name, + symbol, + owner.address, + originChainId, + ]) return { owner, minter, recipient, user, evil, bridge, pToken } } - it('Should mint some pTokens', async () => { - const { owner, minter, recipient, pToken } = await loadFixture(setup) + if (_tokenKind.includes('PToken')) { + it('Should mint some pTokens before upgrade', async () => { + const { owner, minter, recipient, pToken } = await loadFixture(setup) - const value = 100 - await expect(pToken.connect(owner).grantMinterRole(minter)).to.emit( - pToken, - 'RoleGranted', - ) - await expect(pToken.connect(minter).mint(recipient, value)) - .to.emit(pToken, 'Transfer') - .withArgs(ZeroAddress, recipient.address, value) + const value = 100 + await expect(pToken.connect(owner).grantMinterRole(minter)).to.emit( + pToken, + 'RoleGranted', + ) + await expect(pToken.connect(minter).mint(recipient, value)) + .to.emit(pToken, 'Transfer') + .withArgs(ZeroAddress, recipient.address, value) - expect(await pToken.balanceOf(recipient)).to.be.equal(value) - }) + expect(await pToken.balanceOf(recipient)).to.be.equal(value) + }) - it('Should upgrade the ptoken correctly', async () => { - const { owner, minter, recipient, pToken } = await loadFixture(setup) - const value = 100 - await pToken.connect(owner).grantMinterRole(minter) - await pToken.connect(minter).mint(recipient, value) + it('Should upgrade the ptoken correctly', async () => { + const { owner, minter, recipient, pToken } = await loadFixture(setup) + const value = 100 + await pToken.connect(owner).grantMinterRole(minter) + await pToken.connect(minter).mint(recipient, value) - const opts = getUpgradeOpts(owner, _useGSN) + const opts = getUpgradeOpts(owner, _useGSN) - const pTokenV2 = await upgradeProxy( - hre, - pToken, - `PTokenV2${_useGSN}`, - opts, - ) + const pTokenV2 = await upgradeProxy( + hre, + pToken, + `PTokenV2${_useGSN}`, + opts, + ) - expect(await pTokenV2.balanceOf(recipient)).to.be.equal(100) - }) + expect(await pTokenV2.balanceOf(recipient)).to.be.equal(100) + }) + } - describe('Cumulative tests after pToken contract upgrade', () => { + describe('Cumulative tests after pToken contract upgrade or when using XERC20', () => { let owner, admin, + user, minter, recipient, - user, evil, PAM, bridge, @@ -138,44 +150,66 @@ const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) const opts = getUpgradeOpts(owner, _useGSN) - pTokenV2 = await upgradeProxy(hre, pToken, `PTokenV2${_useGSN}`, opts) + pTokenV2 = _tokenKind.includes('PToken') + ? await upgradeProxy(hre, pToken, `PTokenV2${_useGSN}`, opts) + : pToken expect(await pTokenV2.owner()).to.be.equal(owner.address) }) - it('Only the owner can set the fee manager the first time', async () => { + it.only('Anyone can set the fee manager the first time', async () => { feesManagerTest = await deploy(hre, 'FeesManagerTest') - await expect( - pTokenV2.connect(evil).setFeesManager(feesManagerTest), - ).to.be.revertedWith('OnlyOwner') - - await expect(pTokenV2.connect(owner).setFeesManager(feesManagerTest)) + await expect(pTokenV2.connect(user).setFeesManager(feesManagerTest)) .to.emit(pTokenV2, 'FeesManagerChanged') .withArgs(feesManagerTest) + + expect(await pTokenV2.getFeesManager()).to.be.equal(feesManagerTest) }) - it('Only the fees manager can set the fee manager after the first time', async () => { - await expect( - pTokenV2.connect(owner).setFeesManager(feesManagerTest), - ).to.be.revertedWith('OnlyFeesManager') + it.only('Only the fees manager can set the fee manager after the first time', async () => { + const oldFeesManager = feesManagerTest + + feesManagerTest = await deploy(hre, 'FeesManagerTest') + + const tx = pTokenV2.connect(owner).setFeesManager(feesManagerTest) + + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'OnlyFeesManager', + ) + } else { + await expect(tx).to.be.revertedWith('OnlyFeesManager') + } await expect( - feesManagerTest.setFeesManagerForXERC20(pTokenV2, feesManagerTest), + oldFeesManager.setFeesManagerForXERC20(pTokenV2, feesManagerTest), ) .to.emit(pTokenV2, 'FeesManagerChanged') .withArgs(feesManagerTest) + + expect(await pTokenV2.getFeesManager()).to.be.equal(feesManagerTest) }) - it('Only the owner can set limits', async () => { + it.only('Only the owner can set limits', async () => { const mintingLimit = 200 const burningLimit = 300 - await expect( - pTokenV2 - .connect(evil) - .setLimits(bridge, mintingLimit, burningLimit), - ).to.be.revertedWith('Ownable: caller is not the owner') + const tx = pTokenV2 + .connect(evil) + .setLimits(bridge, mintingLimit, burningLimit) + + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'OwnableUnauthorizedAccount', + ) + } else { + await expect(tx).to.be.revertedWith( + 'Ownable: caller is not the owner', + ) + } await expect( pTokenV2 @@ -193,20 +227,35 @@ const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) ) }) - it('Only the allowed bridge can mint and burn', async () => { + it.only('Only the allowed bridge can mint and burn', async () => { const value = 100 - await expect( - pTokenV2.connect(evil).mint(recipient, value), - ).to.be.revertedWith('IXERC20_NotHighEnoughLimits') + + let tx = pTokenV2.connect(evil).mint(recipient, value) + + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'IXERC20_NotHighEnoughLimits', + ) + } else { + await expect(tx).to.be.revertedWith('IXERC20_NotHighEnoughLimits') + } // Sent to evil in order to make the next assertion await expect(pTokenV2.connect(bridge).mint(evil, value)) .to.emit(pTokenV2, 'Transfer') .withArgs(ZeroAddress, evil.address, value) - await expect( - pTokenV2.connect(evil)['burn(address,uint256)'](evil, value), - ).to.be.revertedWith('IXERC20_NotHighEnoughLimits') + tx = pTokenV2.connect(evil)['burn(address,uint256)'](evil, value) + + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'IXERC20_NotHighEnoughLimits', + ) + } else { + await expect(tx).to.be.revertedWith('IXERC20_NotHighEnoughLimits') + } await pTokenV2.connect(evil).approve(bridge, value) await expect( @@ -216,23 +265,32 @@ const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) .withArgs(evil, ZeroAddress, value) }) - it('Only the owner can set the PAM address', async () => { + it.only('Only the owner can set the PAM address', async () => { PAM = await deploy(hre, 'PAM') - await expect( - pTokenV2.connect(evil).setPAM(bridge, PAM), - ).to.be.revertedWith('Ownable: caller is not the owner') + const tx = pTokenV2.connect(evil).setPAM(bridge, PAM) + + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'OwnableUnauthorizedAccount', + ) + } else { + await expect(tx).to.be.revertedWith( + 'Ownable: caller is not the owner', + ) + } await expect(pTokenV2.connect(owner).setPAM(bridge, PAM)) .to.emit(pTokenV2, 'PAMChanged') .withArgs(PAM) }) - it('Should return false when the lockbox is not set', async () => { + it.only('Should return false when the lockbox is not set', async () => { expect(await pTokenV2.isLocal()).to.be.equal(false) }) - it('Only owner can set the lockbox', async () => { + it.only('Only owner can set the lockbox', async () => { const isNative = false const erc20 = ZeroAddress const xerc20 = pTokenV2 @@ -242,20 +300,28 @@ const deployERC1820 = () => helpers.setCode(ERC1820, ERC1820BYTES) isNative, ]) - await expect( - pTokenV2.connect(evil).setLockbox(lockbox), - ).to.be.revertedWith('Ownable: caller is not the owner') + const tx = pTokenV2.connect(evil).setLockbox(lockbox) + if (_tokenKind === 'XERC20') { + await expect(tx).to.be.revertedWithCustomError( + pTokenV2, + 'OwnableUnauthorizedAccount', + ) + } else { + await expect(tx).to.be.revertedWith( + 'Ownable: caller is not the owner', + ) + } await expect(pTokenV2.connect(owner).setLockbox(lockbox)) .to.emit(pTokenV2, 'LockboxSet') .withArgs(lockbox) }) - it('Should return true when the lockbox is set', async () => { + it.only('Should return true when the lockbox is set', async () => { expect(await pTokenV2.isLocal()).to.be.equal(true) }) - it('Should read storage correctly', async () => { + it.only('Should read storage correctly', async () => { expect(await pTokenV2.getLockbox()).to.be.equal(lockbox) expect(await pTokenV2.getPAM(bridge.address)).to.be.equal(PAM) expect(await pTokenV2.getFeesManager()).to.be.equal(feesManagerTest)