feat: Instant Rewards contract audit suggestions (#177)

* feat: Instant Rewards contract audit suggestions * update to solidity 8.20
zeta-chain · Sep 2, 2024 · 96809eb · 96809eb
1 parent fbdcc0c
commit 96809eb
Show file tree

Hide file tree

Showing 8 changed files with 261 additions and 133 deletions.
diff --git a/package.json b/package.json
@@ -29,7 +29,7 @@
   "devDependencies": {
     "@changesets/cli": "^2.23.1",
     "@nomicfoundation/hardhat-verify": "2.0.3",
-    "@nomiclabs/hardhat-ethers": "^2.0.5",
+    "@nomiclabs/hardhat-ethers": "^2.2.3",
     "@nomiclabs/hardhat-waffle": "^2.0.3",
     "@typechain/ethers-v5": "^10.0.0",
     "@typechain/hardhat": "^6.0.0",
@@ -55,8 +55,8 @@
     "eslint-plugin-typescript-sort-keys": "2.1.0",
     "ethereum-waffle": "^4.0.9",
     "ethereumjs-utils": "^5.2.5",
-    "ethers": "5.6.8",
-    "hardhat": "2.12.6",
+    "ethers": "5.7.2",
+    "hardhat": "2.22.6",
     "inquirer": "^8.2.4",
     "mocha": "^10.2.0",
     "ts-mocha": "^10.0.0",

diff --git a/packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol b/packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
@@ -1,22 +1,19 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.7;
+pragma solidity ^0.8.20;
 
 import "@openzeppelin/contracts/security/Pausable.sol";
-import "@openzeppelin/contracts/access/Ownable.sol";
-import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
+import "@openzeppelin/contracts/access/Ownable2Step.sol";
 import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+import "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
+import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";
 
-contract InstantRewards is Ownable, Pausable, ReentrancyGuard {
-    /* An ECDSA signature. */
-    struct Signature {
-        uint8 v;
-        bytes32 r;
-        bytes32 s;
-    }
+contract InstantRewards is Ownable2Step, Pausable, ReentrancyGuard, EIP712 {
+    bytes32 private constant CLAIM_TYPEHASH =
+        keccak256("Claim(address to,uint256 sigExpiration,bytes32 taskId,uint256 amount)");
 
     struct ClaimData {
         address to;
-        Signature signature;
+        bytes signature;
         uint256 sigExpiration;
         bytes32 taskId;
         uint256 amount;
@@ -26,6 +23,7 @@ contract InstantRewards is Ownable, Pausable, ReentrancyGuard {
 
     address public signerAddress;
 
+    event SignerUpdated(address indexed signerAddress);
     event Claimed(address indexed to, bytes32 indexed taskId, uint256 amount);
     event Withdrawn(address indexed wallet, uint256 amount);
 
@@ -35,41 +33,26 @@ contract InstantRewards is Ownable, Pausable, ReentrancyGuard {
     error TaskAlreadyClaimed();
     error TransferFailed();
 
-    constructor(address signerAddress_, address owner) Ownable() {
+    constructor(address signerAddress_, address owner) Ownable() EIP712("InstantRewards", "1") {
         if (signerAddress_ == address(0)) revert InvalidAddress();
         transferOwnership(owner);
         signerAddress = signerAddress_;
     }
 
     function _verify(ClaimData memory claimData) private view {
-        bytes32 payloadHash = _calculateHash(claimData);
-
-        bytes32 messageHash = ECDSA.toEthSignedMessageHash(payloadHash);
-
-        address messageSigner = ECDSA.recover(
-            messageHash,
-            claimData.signature.v,
-            claimData.signature.r,
-            claimData.signature.s
+        bytes32 structHash = keccak256(
+            abi.encode(CLAIM_TYPEHASH, claimData.to, claimData.sigExpiration, claimData.taskId, claimData.amount)
         );
+        bytes32 constructedHash = _hashTypedDataV4(structHash);
 
-        if (signerAddress != messageSigner) revert InvalidSigner();
-        if (block.timestamp > claimData.sigExpiration) revert SignatureExpired();
-    }
-
-    // Function to compute the hash of the data and tasks for a token
-    function _calculateHash(ClaimData memory claimData) private pure returns (bytes32) {
-        bytes memory encodedData = abi.encode(
-            claimData.to,
-            claimData.sigExpiration,
-            claimData.taskId,
-            claimData.amount
-        );
+        if (!SignatureChecker.isValidSignatureNow(signerAddress, constructedHash, claimData.signature)) {
+            revert InvalidSigner();
+        }
 
-        return keccak256(encodedData);
+        if (block.timestamp > claimData.sigExpiration) revert SignatureExpired();
     }
 
-    function claim(ClaimData memory claimData) external whenNotPaused nonReentrant {
+    function claim(ClaimData memory claimData) external nonReentrant whenNotPaused {
         claimData.to = msg.sender;
         _verify(claimData);
 
@@ -86,12 +69,15 @@ contract InstantRewards is Ownable, Pausable, ReentrancyGuard {
     function setSignerAddress(address signerAddress_) external onlyOwner {
         if (signerAddress_ == address(0)) revert InvalidAddress();
         signerAddress = signerAddress_;
+        emit SignerUpdated(signerAddress_);
     }
 
     function withdraw(address wallet, uint256 amount) external onlyOwner {
         if (wallet == address(0)) revert InvalidAddress();
         if (amount > address(this).balance) revert TransferFailed();
-        payable(wallet).transfer(amount);
+        (bool success, ) = wallet.call{value: amount}("");
+        if (!success) revert TransferFailed();
+
         emit Withdrawn(wallet, amount);
     }
 

diff --git a/packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol b/packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.9;
+pragma solidity ^0.8.20;
 
 import "../xpNFT.sol";
 

diff --git a/packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol b/packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.9;
+pragma solidity ^0.8.20;
 
 import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
@@ -117,18 +117,6 @@ contract ZetaXP is ERC721Upgradeable, Ownable2StepUpgradeable, EIP712Upgradeable
         if (updateData.sigTimestamp <= lastUpdateTimestampByTokenId[tokenId]) revert OutdatedSignature();
     }
 
-    // Function to compute the hash of the data and tasks for a token
-    function _calculateHash(UpdateData memory updateData) private pure returns (bytes32) {
-        bytes memory encodedData = abi.encode(
-            updateData.to,
-            updateData.signatureExpiration,
-            updateData.sigTimestamp,
-            updateData.tag
-        );
-
-        return keccak256(encodedData);
-    }
-
     function _updateNFT(uint256 tokenId, UpdateData memory updateData) internal {
         _verify(tokenId, updateData);
         lastUpdateTimestampByTokenId[tokenId] = updateData.sigTimestamp;

diff --git a/packages/zevm-app-contracts/package.json b/packages/zevm-app-contracts/package.json
@@ -39,7 +39,6 @@
     "@openzeppelin/hardhat-upgrades": "^1.7.0-rc.0",
     "@uniswap/v2-periphery": "1.1.0-beta.0",
     "@zetachain/networks": "^4.0.0",
-    "@zetachain/protocol-contracts": "^4.0.1",
-    "ethers": "5.6.8"
+    "@zetachain/protocol-contracts": "^4.0.1"
   }
 }
diff --git a/packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts b/packages/zevm-app-contracts/test/instant-rewards/instant-rewards.ts
@@ -7,6 +7,8 @@ import { ethers } from "hardhat";
 import { InstantRewards } from "../../typechain-types";
 import { ClaimData, getSignature } from "./test.helpers";
 
+const HARDHAT_CHAIN_ID = 1337;
+
 describe("Instant Rewards Contract test", () => {
   let instantRewards: InstantRewards,
     owner: SignerWithAddress,
@@ -17,6 +19,8 @@ describe("Instant Rewards Contract test", () => {
   const encodeTaskId = (taskId: string) => utils.keccak256(utils.defaultAbiCoder.encode(["string"], [taskId]));
 
   const getClaimDataSigned = async (
+    chainId: number,
+    verifyingContract: string,
     signer: SignerWithAddress,
     amount: BigNumber,
     sigExpiration: number,
@@ -30,7 +34,7 @@ describe("Instant Rewards Contract test", () => {
       to,
     };
 
-    const signature = await getSignature(signer, claimData);
+    const signature = await getSignature(chainId, verifyingContract, signer, claimData);
     return {
       ...claimData,
       signature,
@@ -59,7 +63,15 @@ describe("Instant Rewards Contract test", () => {
       value: amount,
     });
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.emit(instantRewards, "Claimed").withArgs(owner.address, taskId, amount);
@@ -81,7 +93,15 @@ describe("Instant Rewards Contract test", () => {
       value: amount,
     });
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.emit(instantRewards, "Claimed").withArgs(owner.address, taskId, amount);
@@ -94,7 +114,15 @@ describe("Instant Rewards Contract test", () => {
     const taskId = encodeTaskId("WALLET/TASK/EPOC");
     const to = user.address;
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.revertedWith("InvalidSigner");
@@ -107,7 +135,15 @@ describe("Instant Rewards Contract test", () => {
     const taskId = encodeTaskId("WALLET/TASK/EPOC");
     const to = owner.address;
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.revertedWith("SignatureExpired");
@@ -122,7 +158,15 @@ describe("Instant Rewards Contract test", () => {
 
     await instantRewards.pause();
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.revertedWith("Pausable: paused");
@@ -141,7 +185,15 @@ describe("Instant Rewards Contract test", () => {
       value: amount,
     });
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
     instantRewards.claim(claimDataSigned);
 
@@ -163,10 +215,26 @@ describe("Instant Rewards Contract test", () => {
     });
 
     {
-      const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+      const claimDataSigned = await getClaimDataSigned(
+        HARDHAT_CHAIN_ID,
+        instantRewards.address,
+        signer,
+        amount,
+        sigExpiration,
+        taskId,
+        to
+      );
       instantRewards.claim(claimDataSigned);
     }
-    const claimDataSigned = await getClaimDataSigned(signer, amount.add(parseEther("1")), sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount.add(parseEther("1")),
+      sigExpiration,
+      taskId,
+      to
+    );
     const tx = instantRewards.claim(claimDataSigned);
     await expect(tx).to.revertedWith("TaskAlreadyClaimed");
   });
@@ -184,9 +252,25 @@ describe("Instant Rewards Contract test", () => {
       value: amount,
     });
 
-    const claimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration, taskId, to);
+    const claimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration,
+      taskId,
+      to
+    );
 
-    const newClaimDataSigned = await getClaimDataSigned(signer, amount, sigExpiration + 1000, taskId, to);
+    const newClaimDataSigned = await getClaimDataSigned(
+      HARDHAT_CHAIN_ID,
+      instantRewards.address,
+      signer,
+      amount,
+      sigExpiration + 1000,
+      taskId,
+      to
+    );
 
     instantRewards.claim(newClaimDataSigned);
 
@@ -205,6 +289,7 @@ describe("Instant Rewards Contract test", () => {
       expect(ownerAddr).to.be.eq(owner.address);
     }
     await instantRewards.transferOwnership(user.address);
+    await instantRewards.connect(user).acceptOwnership();
     {
       const ownerAddr = await instantRewards.owner();
       expect(ownerAddr).to.be.eq(user.address);