From 484d62672035b3b3559e754b188c61f62d40fa6d Mon Sep 17 00:00:00 2001 From: doylet Date: Fri, 12 Jul 2024 15:57:03 +1000 Subject: [PATCH 1/2] Allow EIP2612, gas-less approvals in the staking flow --- contracts/SENT.sol | 5 +- contracts/ServiceNodeContribution.sol | 25 +++++- contracts/ServiceNodeRewards.sol | 17 +++- contracts/test/MockERC20.sol | 5 +- hardhat.config.js | 3 + test/unit-js/ServiceNodeContributionTest.js | 98 +++++++++++++++++++++ test/unit-js/ServiceNodeRewardsTest.js | 1 - 7 files changed, 147 insertions(+), 7 deletions(-) diff --git a/contracts/SENT.sol b/contracts/SENT.sol index 6860ed9..b413c06 100644 --- a/contracts/SENT.sol +++ b/contracts/SENT.sol @@ -2,17 +2,18 @@ pragma solidity ^0.8.20; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; import "./libraries/Shared.sol"; /** * @title SENT contract * @notice The SENT utility token */ -contract SENT is ERC20, Shared { +contract SENT is ERC20, ERC20Permit, Shared { constructor( uint256 totalSupply_, address receiverGenesisAddress - ) ERC20("Session", "SENT") nzAddr(receiverGenesisAddress) nzUint(totalSupply_) { + ) ERC20("Session", "SENT") ERC20Permit("Session") nzAddr(receiverGenesisAddress) nzUint(totalSupply_) { _mint(receiverGenesisAddress, totalSupply_); } diff --git a/contracts/ServiceNodeContribution.sol b/contracts/ServiceNodeContribution.sol index 3739cf8..7589da4 100644 --- a/contracts/ServiceNodeContribution.sol +++ b/contracts/ServiceNodeContribution.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.20; import "./libraries/Shared.sol"; import "./interfaces/IServiceNodeRewards.sol"; +import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; /** @@ -93,7 +94,6 @@ contract ServiceNodeContribution is Shared { // State-changing functions // // // ////////////////////////////////////////////////////////////// - /** * @notice Allows the operator to contribute funds towards their own node. * @@ -120,6 +120,18 @@ contract ServiceNodeContribution is Shared { contributeFunds(amount); } + function contributeOperatorFundsWithPermit( + uint256 amount, + IServiceNodeRewards.BLSSignatureParams memory _blsSignature, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) public onlyOperator { + IERC20Permit(address(SENT)).permit(msg.sender, address(this), amount, deadline, v, r, s); + contributeOperatorFunds(amount, _blsSignature); + } + /** * @notice Contribute funds to the contract for the service node run by * `operator`. The `amount` of SENT token must be at least the @@ -179,6 +191,17 @@ contract ServiceNodeContribution is Shared { } } + function contributeFundsWithPermit( + uint256 amount, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) public { + IERC20Permit(address(SENT)).permit(msg.sender, address(this), amount, deadline, v, r, s); + contributeFunds(amount); + } + /** * @notice Invoked when the `totalContribution` of the contract matches the * `stakingRequirement`. The service node registration and SENT tokens are diff --git a/contracts/ServiceNodeRewards.sol b/contracts/ServiceNodeRewards.sol index e802f15..ebea259 100644 --- a/contracts/ServiceNodeRewards.sol +++ b/contracts/ServiceNodeRewards.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.20; import "./interfaces/IServiceNodeRewards.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; @@ -285,10 +286,24 @@ contract ServiceNodeRewards is Initializable, Ownable2StepUpgradeable, PausableU BLSSignatureParams calldata blsSignature, ServiceNodeParams calldata serviceNodeParams, Contributor[] calldata contributors - ) external whenNotPaused { + ) public whenNotPaused { _addBLSPublicKey(blsPubkey, blsSignature, msg.sender, serviceNodeParams, contributors); } + function addBLSPublicKeyWithPermit( + BN256G1.G1Point calldata blsPubkey, + BLSSignatureParams calldata blsSignature, + ServiceNodeParams calldata serviceNodeParams, + Contributor[] calldata contributors, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) external whenNotPaused { + IERC20Permit(address(designatedToken)).permit(msg.sender, address(this), _stakingRequirement, deadline, v, r, s); + addBLSPublicKey(blsPubkey, blsSignature, serviceNodeParams, contributors); + } + /// @dev Internal function to add a BLS public key. /// /// @param blsPubkey 64 byte BLS public key for the service node. diff --git a/contracts/test/MockERC20.sol b/contracts/test/MockERC20.sol index e1eeec1..71d7cbb 100644 --- a/contracts/test/MockERC20.sol +++ b/contracts/test/MockERC20.sol @@ -2,10 +2,11 @@ pragma solidity ^0.8.20; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; -contract MockERC20 is ERC20 { +contract MockERC20 is ERC20, ERC20Permit { uint8 immutable d; - constructor(string memory name_, string memory symbol_, uint8 _decimals) ERC20(name_, symbol_) { + constructor(string memory name_, string memory symbol_, uint8 _decimals) ERC20(name_, symbol_) ERC20Permit(name_) { d = _decimals; _mint(msg.sender, 1e8 * (10 ** uint256(d))); // Mint 100 million tokens for testing } diff --git a/hardhat.config.js b/hardhat.config.js index b0ad009..3785978 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -35,5 +35,8 @@ module.exports = { }, }, }, + paths: { + tests: "./test/unit-js", + }, }; diff --git a/test/unit-js/ServiceNodeContributionTest.js b/test/unit-js/ServiceNodeContributionTest.js index f990c2f..7f3d66e 100644 --- a/test/unit-js/ServiceNodeContributionTest.js +++ b/test/unit-js/ServiceNodeContributionTest.js @@ -57,6 +57,36 @@ async function withdrawContributor(sentToken, snContribution, contributor) { expect(contributorArrayExpected).to.deep.equal(contributorArray); } +async function setupPermitSignature(token, owner, spenderAddress, amount, deadline) { + const domain = { + name: await token.name(), + version: '1', + chainId: (await ethers.provider.getNetwork()).chainId, + verifyingContract: await token.getAddress(), + }; + + const schema = { + Permit: [ + { name: 'owner', type: 'address' }, + { name: 'spender', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint256' }, + ] + }; + + const payload = { + owner: owner.address, + spender: spenderAddress, + value: amount, + nonce: await token.nonces(owner.address), + deadline: deadline, + }; + + const result = await owner.signTypedData(domain, schema, payload); + return result; +} + describe("ServiceNodeContribution Contract Tests", function () { // NOTE: Contract factories for deploying onto the blockchain let sentTokenContractFactory; @@ -258,6 +288,74 @@ describe("ServiceNodeContribution Contract Tests", function () { .to.be.revertedWith("Contribution is below minimum requirement"); }); + it("Operator can contribute stake w/ permit", async function () { + const stake = await snContribution.stakingRequirement(); + + const currentBlock = await ethers.provider.getBlockNumber(); + const blockTimestamp = (await ethers.provider.getBlock(currentBlock)).timestamp; + const deadline = blockTimestamp + (60 * 10); // Expire 10 minutes from now + + const signature = await setupPermitSignature(sentToken, snOperator, snContributionAddress, stake, deadline); + const signatureNo0x = signature.substring(2); + const r = signatureNo0x.substring(0, 64); + const s = signatureNo0x.substring(64, 128); + const v = signatureNo0x.substring(128, 130); + await snContribution.connect(snOperator) + .contributeOperatorFundsWithPermit(stake, + [3, 4, 5, 6], + deadline, + '0x' + v, + '0x' + r, + '0x' + s); + expect(await snContribution.totalContribution()).to.equal(stake); + }); + + it("Operator can't contribute more than staking requirement w/ permit", async function () { + const stake = (await snContribution.stakingRequirement()) + BigInt(1); + + const currentBlock = await ethers.provider.getBlockNumber(); + const blockTimestamp = (await ethers.provider.getBlock(currentBlock)).timestamp; + const deadline = blockTimestamp + (60 * 10); // Expire 10 minutes from now + + const signature = await setupPermitSignature(sentToken, snOperator, snContributionAddress, stake, deadline); + const signatureNo0x = signature.substring(2); + const r = signatureNo0x.substring(0, 64); + const s = signatureNo0x.substring(64, 128); + const v = signatureNo0x.substring(128, 130); + await expect(snContribution.connect(snOperator) + .contributeOperatorFundsWithPermit(stake, + [3, 4, 5, 6], + deadline, + '0x' + v, + '0x' + r, + '0x' + s)).to.be.reverted; + expect(await snContribution.totalContribution()).to.equal(BigInt(0)); + }); + + + it("Operator can't submit expired stake w/ permit", async function () { + const stake = await snContribution.stakingRequirement(); + await sentToken.transfer(snOperator, stake); + + const currentBlock = await ethers.provider.getBlockNumber(); + const blockTimestamp = (await ethers.provider.getBlock(currentBlock)).timestamp; + const deadline = blockTimestamp - (60 * 10); // Expired 10 minutes prior + + const signature = await setupPermitSignature(sentToken, snOperator, snContributionAddress, stake, deadline); + const signatureNo0x = signature.substring(2); + const r = signatureNo0x.substring(0, 64); + const s = signatureNo0x.substring(64, 128); + const v = signatureNo0x.substring(128, 130); + await expect(snContribution.connect(snOperator) + .contributeOperatorFundsWithPermit(stake, + [3, 4, 5, 6], + deadline, + '0x' + v, + '0x' + r, + '0x' + s)).to.be.reverted; + expect(await snContribution.totalContribution()).to.equal(0); + }); + it("Allows operator to contribute and records correct balance", async function () { const minContribution = await snContribution.minimumContribution(); await sentToken.transfer(snOperator, TEST_AMNT); diff --git a/test/unit-js/ServiceNodeRewardsTest.js b/test/unit-js/ServiceNodeRewardsTest.js index 30c699a..5995ddc 100644 --- a/test/unit-js/ServiceNodeRewardsTest.js +++ b/test/unit-js/ServiceNodeRewardsTest.js @@ -105,7 +105,6 @@ describe("ServiceNodeRewards Contract Tests", function () { await expect(serviceNodeRewards.connect(owner).seedPublicKeyList(pkX, pkY, amounts)) .to.be.revertedWithCustomError(serviceNodeRewards, "BLSPubkeyAlreadyExists") }); - }); }); From 2f89875c6751a5f452456e42dab2ee53fb234e3b Mon Sep 17 00:00:00 2001 From: doylet Date: Mon, 15 Jul 2024 10:53:56 +1000 Subject: [PATCH 2/2] Add try-catch guard around permit to be resistant to front-running --- contracts/ServiceNodeContribution.sol | 10 ++++++++-- contracts/ServiceNodeRewards.sol | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/contracts/ServiceNodeContribution.sol b/contracts/ServiceNodeContribution.sol index 7589da4..df6df1a 100644 --- a/contracts/ServiceNodeContribution.sol +++ b/contracts/ServiceNodeContribution.sol @@ -128,7 +128,10 @@ contract ServiceNodeContribution is Shared { bytes32 r, bytes32 s ) public onlyOperator { - IERC20Permit(address(SENT)).permit(msg.sender, address(this), amount, deadline, v, r, s); + // NOTE: Try catch makes the code tolerant to front-running, see: + // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/05f218fb6617932e56bf5388c3b389c3028a7b73/contracts/token/ERC20/extensions/IERC20Permit.sol#L19 + IERC20Permit token = IERC20Permit(address(SENT)); + try token.permit(msg.sender, address(this), amount, deadline, v, r, s) {} catch {} contributeOperatorFunds(amount, _blsSignature); } @@ -198,7 +201,10 @@ contract ServiceNodeContribution is Shared { bytes32 r, bytes32 s ) public { - IERC20Permit(address(SENT)).permit(msg.sender, address(this), amount, deadline, v, r, s); + // NOTE: Try catch makes the code tolerant to front-running, see: + // `contributeOperatorFundsWithPermit` + IERC20Permit token = IERC20Permit(address(SENT)); + try token.permit(msg.sender, address(this), amount, deadline, v, r, s) {} catch {} contributeFunds(amount); } diff --git a/contracts/ServiceNodeRewards.sol b/contracts/ServiceNodeRewards.sol index ebea259..130a9b9 100644 --- a/contracts/ServiceNodeRewards.sol +++ b/contracts/ServiceNodeRewards.sol @@ -300,7 +300,10 @@ contract ServiceNodeRewards is Initializable, Ownable2StepUpgradeable, PausableU bytes32 r, bytes32 s ) external whenNotPaused { - IERC20Permit(address(designatedToken)).permit(msg.sender, address(this), _stakingRequirement, deadline, v, r, s); + // NOTE: Try catch makes the code tolerant to front-running, see: + // ServiceNodeContribution.sol + IERC20Permit token = IERC20Permit(address(designatedToken)); + try token.permit(msg.sender, address(this), _stakingRequirement, deadline, v, r, s) {} catch {} addBLSPublicKey(blsPubkey, blsSignature, serviceNodeParams, contributors); }