From 321cc52e4419f2042135835a73bd48d5987c4958 Mon Sep 17 00:00:00 2001 From: sprtd Date: Mon, 10 Apr 2023 11:53:43 +0100 Subject: [PATCH 1/3] feat: create CohortBank contract --- README.md | 6 ----- contracts/CohortBank.sol | 58 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 contracts/CohortBank.sol diff --git a/README.md b/README.md index 1933d03..4a9515a 100644 --- a/README.md +++ b/README.md @@ -2,10 +2,4 @@ This repository contains a collection of Solidity examples that students can use to learn about smart contracts and blockchain development. -## Examples -This repository contains the following Solidity examples: - -- [Storage.sol](./001_Storage.sol) - A simple smart contract that demonstrates various Solidity data types and modifiers. - -- [Mapping.sol](./002_Mapping.sol) - A simple smart contract that demonstrates the use of the mapping data structure. \ No newline at end of file diff --git a/contracts/CohortBank.sol b/contracts/CohortBank.sol new file mode 100644 index 0000000..477be51 --- /dev/null +++ b/contracts/CohortBank.sol @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.9; + +// Uncomment this line to use console.log +// import "hardhat/console.sol"; + +contract CohortBank { + // The keyword "public" makes variables + // accessible from other contracts + uint256 public unlockTime; + mapping(address => uint256) public balances; + uint256 public totalCohortBalance; + + address payable public owner; + // Events allow clients to react to specific + // contract changes you declare + event Deposit(uint256 amount, uint256 when, address caller); + event Withdrawal(uint256 amount, uint256 when); + + // Constructor code is only run when the contract + // is created + constructor(uint256 _unlockTime) payable { + require( + block.timestamp < _unlockTime, + "Unlock time should be in the future" + ); + + unlockTime = _unlockTime; + owner = payable(msg.sender); + } + + + // spot the error here + function deposit() public payable { + // Uncomment this line, and the import of "hardhat/console.sol", to print a log in your terminal + // console.log("Unlock time is %o and block timestamp is %o", unlockTime, block.timestamp); + require(msg.value > 0, "cannot deposit 0 amount"); + balances[msg.sender] += msg.value; + totalCohortBalance += msg.value; + + emit Deposit(msg.value, block.timestamp, msg.sender); + } + + // buggy withdraw function + // spot this bug + // add your findings to findings.md + function withdraw() public { + // Uncomment this line, and the import of "hardhat/console.sol", to print a log in your terminal + // console.log("Unlock time is %o and block timestamp is %o", unlockTime, block.timestamp); + + require(block.timestamp >= unlockTime, "You can't withdraw yet"); + require(msg.sender == owner, "You aren't the owner"); + + emit Withdrawal(address(this).balance, block.timestamp); + + owner.transfer(address(this).balance); + } +} \ No newline at end of file From 58313497e50fb5d0d7a9f077e03664236472096b Mon Sep 17 00:00:00 2001 From: Mac Date: Fri, 14 Apr 2023 11:12:33 -0500 Subject: [PATCH 2/3] test/cohortBank --- contracts/CohortBank.sol | 17 +-- test/Lock.js | 252 +++++++++++++++++++-------------------- test/Storage.js | 114 +++++++++--------- test/cohortBank.js | 134 +++++++++++++++++++++ 4 files changed, 322 insertions(+), 195 deletions(-) create mode 100644 test/cohortBank.js diff --git a/contracts/CohortBank.sol b/contracts/CohortBank.sol index 477be51..418f2b5 100644 --- a/contracts/CohortBank.sol +++ b/contracts/CohortBank.sol @@ -29,9 +29,8 @@ contract CohortBank { owner = payable(msg.sender); } - - // spot the error here - function deposit() public payable { + // spot the error here + function deposit() public payable { // Uncomment this line, and the import of "hardhat/console.sol", to print a log in your terminal // console.log("Unlock time is %o and block timestamp is %o", unlockTime, block.timestamp); require(msg.value > 0, "cannot deposit 0 amount"); @@ -41,18 +40,12 @@ contract CohortBank { emit Deposit(msg.value, block.timestamp, msg.sender); } - // buggy withdraw function - // spot this bug - // add your findings to findings.md - function withdraw() public { - // Uncomment this line, and the import of "hardhat/console.sol", to print a log in your terminal - // console.log("Unlock time is %o and block timestamp is %o", unlockTime, block.timestamp); - + function withdraw(uint256 amount) public { require(block.timestamp >= unlockTime, "You can't withdraw yet"); require(msg.sender == owner, "You aren't the owner"); - emit Withdrawal(address(this).balance, block.timestamp); + emit Withdrawal(amount, block.timestamp); owner.transfer(address(this).balance); } -} \ No newline at end of file +} diff --git a/test/Lock.js b/test/Lock.js index 6a161e6..49e46f1 100644 --- a/test/Lock.js +++ b/test/Lock.js @@ -1,126 +1,126 @@ -const { - time, - loadFixture, -} = require("@nomicfoundation/hardhat-network-helpers"); -const { anyValue } = require("@nomicfoundation/hardhat-chai-matchers/withArgs"); -const { expect } = require("chai"); - -describe("Lock", function () { - // We define a fixture to reuse the same setup in every test. - // We use loadFixture to run this setup once, snapshot that state, - // and reset Hardhat Network to that snapshot in every test. - async function deployOneYearLockFixture() { - const ONE_YEAR_IN_SECS = 365 * 24 * 60 * 60; - const ONE_GWEI = 1_000_000_000; - - const lockedAmount = ONE_GWEI; - const unlockTime = (await time.latest()) + ONE_YEAR_IN_SECS; - - // Contracts are deployed using the first signer/account by default - const [owner, otherAccount] = await ethers.getSigners(); - - const Lock = await ethers.getContractFactory("Lock"); - const lock = await Lock.deploy(unlockTime, { value: lockedAmount }); - - return { lock, unlockTime, lockedAmount, owner, otherAccount }; - } - - describe("Deployment", function () { - it("Should set the right unlockTime", async function () { - const { lock, unlockTime } = await loadFixture(deployOneYearLockFixture); - - expect(await lock.unlockTime()).to.equal(unlockTime); - }); - - it("Should set the right owner", async function () { - const { lock, owner } = await loadFixture(deployOneYearLockFixture); - - expect(await lock.owner()).to.equal(owner.address); - }); - - it("Should receive and store the funds to lock", async function () { - const { lock, lockedAmount } = await loadFixture( - deployOneYearLockFixture - ); - - expect(await ethers.provider.getBalance(lock.address)).to.equal( - lockedAmount - ); - }); - - it("Should fail if the unlockTime is not in the future", async function () { - // We don't use the fixture here because we want a different deployment - const latestTime = await time.latest(); - const Lock = await ethers.getContractFactory("Lock"); - await expect(Lock.deploy(latestTime, { value: 1 })).to.be.revertedWith( - "Unlock time should be in the future" - ); - }); - }); - - describe("Withdrawals", function () { - describe("Validations", function () { - it("Should revert with the right error if called too soon", async function () { - const { lock } = await loadFixture(deployOneYearLockFixture); - - await expect(lock.withdraw()).to.be.revertedWith( - "You can't withdraw yet" - ); - }); - - it("Should revert with the right error if called from another account", async function () { - const { lock, unlockTime, otherAccount } = await loadFixture( - deployOneYearLockFixture - ); - - // We can increase the time in Hardhat Network - await time.increaseTo(unlockTime); - - // We use lock.connect() to send a transaction from another account - await expect(lock.connect(otherAccount).withdraw()).to.be.revertedWith( - "You aren't the owner" - ); - }); - - it("Shouldn't fail if the unlockTime has arrived and the owner calls it", async function () { - const { lock, unlockTime } = await loadFixture( - deployOneYearLockFixture - ); - - // Transactions are sent using the first signer by default - await time.increaseTo(unlockTime); - - await expect(lock.withdraw()).not.to.be.reverted; - }); - }); - - describe("Events", function () { - it("Should emit an event on withdrawals", async function () { - const { lock, unlockTime, lockedAmount } = await loadFixture( - deployOneYearLockFixture - ); - - await time.increaseTo(unlockTime); - - await expect(lock.withdraw()) - .to.emit(lock, "Withdrawal") - .withArgs(lockedAmount, anyValue); // We accept any value as `when` arg - }); - }); - - describe("Transfers", function () { - it("Should transfer the funds to the owner", async function () { - const { lock, unlockTime, lockedAmount, owner } = await loadFixture( - deployOneYearLockFixture - ); - - await time.increaseTo(unlockTime); - - await expect(lock.withdraw()).to.changeEtherBalances( - [owner, lock], - [lockedAmount, -lockedAmount] - ); - }); - }); - }); -}); +// const { +// time, +// loadFixture, +// } = require("@nomicfoundation/hardhat-network-helpers"); +// const { anyValue } = require("@nomicfoundation/hardhat-chai-matchers/withArgs"); +// const { expect } = require("chai"); + +// describe("Lock", function () { +// // We define a fixture to reuse the same setup in every test. +// // We use loadFixture to run this setup once, snapshot that state, +// // and reset Hardhat Network to that snapshot in every test. +// async function deployOneYearLockFixture() { +// const ONE_YEAR_IN_SECS = 365 * 24 * 60 * 60; +// const ONE_GWEI = 1_000_000_000; + +// const lockedAmount = ONE_GWEI; +// const unlockTime = (await time.latest()) + ONE_YEAR_IN_SECS; + +// // Contracts are deployed using the first signer/account by default +// const [owner, otherAccount] = await ethers.getSigners(); + +// const Lock = await ethers.getContractFactory("Lock"); +// const lock = await Lock.deploy(unlockTime, { value: lockedAmount }); + +// return { lock, unlockTime, lockedAmount, owner, otherAccount }; +// } + +// describe("Deployment", function () { +// it("Should set the right unlockTime", async function () { +// const { lock, unlockTime } = await loadFixture(deployOneYearLockFixture); + +// expect(await lock.unlockTime()).to.equal(unlockTime); +// }); + +// it("Should set the right owner", async function () { +// const { lock, owner } = await loadFixture(deployOneYearLockFixture); + +// expect(await lock.owner()).to.equal(owner.address); +// }); + +// it("Should receive and store the funds to lock", async function () { +// const { lock, lockedAmount } = await loadFixture( +// deployOneYearLockFixture +// ); + +// expect(await ethers.provider.getBalance(lock.address)).to.equal( +// lockedAmount +// ); +// }); + +// it("Should fail if the unlockTime is not in the future", async function () { +// // We don't use the fixture here because we want a different deployment +// const latestTime = await time.latest(); +// const Lock = await ethers.getContractFactory("Lock"); +// await expect(Lock.deploy(latestTime, { value: 1 })).to.be.revertedWith( +// "Unlock time should be in the future" +// ); +// }); +// }); + +// describe("Withdrawals", function () { +// describe("Validations", function () { +// it("Should revert with the right error if called too soon", async function () { +// const { lock } = await loadFixture(deployOneYearLockFixture); + +// await expect(lock.withdraw()).to.be.revertedWith( +// "You can't withdraw yet" +// ); +// }); + +// it("Should revert with the right error if called from another account", async function () { +// const { lock, unlockTime, otherAccount } = await loadFixture( +// deployOneYearLockFixture +// ); + +// // We can increase the time in Hardhat Network +// await time.increaseTo(unlockTime); + +// // We use lock.connect() to send a transaction from another account +// await expect(lock.connect(otherAccount).withdraw()).to.be.revertedWith( +// "You aren't the owner" +// ); +// }); + +// it("Shouldn't fail if the unlockTime has arrived and the owner calls it", async function () { +// const { lock, unlockTime } = await loadFixture( +// deployOneYearLockFixture +// ); + +// // Transactions are sent using the first signer by default +// await time.increaseTo(unlockTime); + +// await expect(lock.withdraw()).not.to.be.reverted; +// }); +// }); + +// describe("Events", function () { +// it("Should emit an event on withdrawals", async function () { +// const { lock, unlockTime, lockedAmount } = await loadFixture( +// deployOneYearLockFixture +// ); + +// await time.increaseTo(unlockTime); + +// await expect(lock.withdraw()) +// .to.emit(lock, "Withdrawal") +// .withArgs(lockedAmount, anyValue); // We accept any value as `when` arg +// }); +// }); + +// describe("Transfers", function () { +// it("Should transfer the funds to the owner", async function () { +// const { lock, unlockTime, lockedAmount, owner } = await loadFixture( +// deployOneYearLockFixture +// ); + +// await time.increaseTo(unlockTime); + +// await expect(lock.withdraw()).to.changeEtherBalances( +// [owner, lock], +// [lockedAmount, -lockedAmount] +// ); +// }); +// }); +// }); +// }); diff --git a/test/Storage.js b/test/Storage.js index 40bb3f9..eb8688b 100644 --- a/test/Storage.js +++ b/test/Storage.js @@ -1,57 +1,57 @@ -const { - time, - loadFixture, -} = require("@nomicfoundation/hardhat-network-helpers"); -const { anyValue } = require("@nomicfoundation/hardhat-chai-matchers/withArgs"); -const { expect } = require("chai"); - -describe("Storage Test Suite", async () => { - const { parseEther, formatEther } = await ethers.utils; - let owner, addr1, addr2, storageContract - beforeEach(async () => { - [owner, addr1, addr2] = await ethers.getSigners(); - const StorageContract = await ethers.getContractFactory("Storage") - storageContract = await StorageContract.deploy(owner.address) - console.log("storage contract address__", storageContract.address) - }) - - describe("Deployment", async () => { - it("should set the right owner", async function () { - console.log("owner ___", owner.address) - expect(await storageContract.owner()).to.equal(owner.address); - expect(await storageContract.owner()).to.not.equal(addr1.address) - expect(await storageContract.owner()).to.not.equal(addr2.address) - }); - }) - - describe("Transactions", async () => { - it("should change number", async function () { - const num1 = 100 - const num2 = 150 - await storageContract.changeNumber(num1) - - const number = await storageContract.number() - expect(number).to.eq(num1) - expect(number).to.not.eq(0) - expect(number).to.not.eq(5) - expect(number).to.not.eq(num2) - - await new Promise(resolve => { - setTimeout(resolve, 2000); // 2s delay - }) - await storageContract.changeNumber(num2) - const number2 = await storageContract.number() - - expect(number2).to.eq(num2) - expect(number2).to.not.eq(num1) - }); - - it("should return myAddress in Storage contract", async function () { - const storedMyAddress = "0x5B38Da6a701c568545dCfcB03FcB875f56beddC4" - expect(await storageContract.viewAddress()).to.eq(storedMyAddress) - expect(await storageContract.viewAddress()).to.not.eq(addr2.address) - }); - }) - - -}); +// const { +// time, +// loadFixture, +// } = require("@nomicfoundation/hardhat-network-helpers"); +// const { anyValue } = require("@nomicfoundation/hardhat-chai-matchers/withArgs"); +// const { expect } = require("chai"); + +// describe("Storage Test Suite", async () => { +// const { parseEther, formatEther } = await ethers.utils; +// let owner, addr1, addr2, storageContract +// beforeEach(async () => { +// [owner, addr1, addr2] = await ethers.getSigners(); +// const StorageContract = await ethers.getContractFactory("Storage") +// storageContract = await StorageContract.deploy(owner.address) +// console.log("storage contract address__", storageContract.address) +// }) + +// describe("Deployment", async () => { +// it("should set the right owner", async function () { +// console.log("owner ___", owner.address) +// expect(await storageContract.owner()).to.equal(owner.address); +// expect(await storageContract.owner()).to.not.equal(addr1.address) +// expect(await storageContract.owner()).to.not.equal(addr2.address) +// }); +// }) + +// describe("Transactions", async () => { +// it("should change number", async function () { +// const num1 = 100 +// const num2 = 150 +// await storageContract.changeNumber(num1) + +// const number = await storageContract.number() +// expect(number).to.eq(num1) +// expect(number).to.not.eq(0) +// expect(number).to.not.eq(5) +// expect(number).to.not.eq(num2) + +// await new Promise(resolve => { +// setTimeout(resolve, 2000); // 2s delay +// }) +// await storageContract.changeNumber(num2) +// const number2 = await storageContract.number() + +// expect(number2).to.eq(num2) +// expect(number2).to.not.eq(num1) +// }); + +// it("should return myAddress in Storage contract", async function () { +// const storedMyAddress = "0x5B38Da6a701c568545dCfcB03FcB875f56beddC4" +// expect(await storageContract.viewAddress()).to.eq(storedMyAddress) +// expect(await storageContract.viewAddress()).to.not.eq(addr2.address) +// }); +// }) + + +// }); diff --git a/test/cohortBank.js b/test/cohortBank.js new file mode 100644 index 0000000..4548dd2 --- /dev/null +++ b/test/cohortBank.js @@ -0,0 +1,134 @@ +const { + time, + loadFixture, + } = require("@nomicfoundation/hardhat-network-helpers"); + const { anyValue } = require("@nomicfoundation/hardhat-chai-matchers/withArgs"); + const { expect } = require("chai"); + const { ethers } = require("hardhat"); + + const TEST_AMOUNT = ethers.utils.parseEther("1"); + + describe("CohortBank", function () { + async function runEveryTime() { + const ONE_YEAR_IN_SECONDS = 365 * 24 * 60 * 60; + const ONE_GWEI = 1000000000; + + const lockedAmount = ONE_GWEI; + const unlockTime = (await time.latest()) + ONE_YEAR_IN_SECONDS; + const [owner, adr1] = await ethers.getSigners(); + + const CohortBank = await ethers.getContractFactory("CohortBank"); + const cohortBank = await CohortBank.deploy(unlockTime, { + value: lockedAmount, + }); + + await cohortBank.deployed(); + + return { cohortBank, owner, adr1, unlockTime, lockedAmount }; + } + + // Deployment + describe("Deployment", function () { + // Checking Unlockedtime + it("Should check unlocked time", async () => { + const { cohortBank, unlockTime } = await loadFixture(runEveryTime); + + expect(await cohortBank.unlockTime()).to.equal(unlockTime); + }); + + // Checking Owner + it("Should set the owner", async () => { + const { cohortBank, owner } = await loadFixture(runEveryTime); + + expect(await cohortBank.owner()).to.equal(owner.address); + }); + + // Check balance + it("Should receive and store the funds to CohortBank", async () => { + const { cohortBank, lockedAmount } = await loadFixture(runEveryTime); + // const totalBAl = await ethers.provider.getBalance(cohortBank.address); + // console.log(totalBAl.toNumber()); + expect(await ethers.provider.getBalance(cohortBank.address)).to.equal( + lockedAmount + ); + }); + + // Condition check + it("Should fail if the unlockTime is not inthe future", async () => { + const latestTime = await time.latest(); + const CohortBank = await ethers.getContractFactory("CohortBank"); + await expect( + CohortBank.deploy(latestTime, { value: 1 }) + ).to.be.revertedWith("Unlock time should be in the future"); + }); + }); + + // Withdrawals + describe("Withdrawals", function () { + describe("Validations", async function () { + it("Should revert if the owner calls before unlock time", async () => { + const { cohortBank, unlockTime, adr1 } = await loadFixture( + runEveryTime + ); + + //make deposit of a certain amount, that will be withdrawn + + await cohortBank.deposit({ value: TEST_AMOUNT }); + await expect(cohortBank.withdraw(TEST_AMOUNT)).to.be.revertedWith( + "You can't withdraw yet" + ); + }); + + it("Shold fail if not called by the owner", async () => { + const { cohortBank, unlockTime, adr1 } = await loadFixture( + runEveryTime + ); + + //make deposit of a certain amount, that will be withdrawn + + await cohortBank.deposit({ value: TEST_AMOUNT }); + await time.increaseTo(unlockTime); + await expect( + cohortBank.connect(adr1).withdraw(TEST_AMOUNT) + ).to.be.revertedWith("You aren't the owner"); + }); + }); + }); + + // Events + describe("Events", function () { + it("Should emit the event on withdrawals", async () => { + const { cohortBank, unlockTime } = await loadFixture(runEveryTime); + await cohortBank.deposit({ value: TEST_AMOUNT }); + + await time.increaseTo(unlockTime); + + await expect(cohortBank.withdraw(TEST_AMOUNT)) + .to.emit(cohortBank, "Withdrawal") + .withArgs(TEST_AMOUNT, anyValue); + }); + }); + + // Transfer + describe("Transfer", function () { + it("Should transfer the funds to the owner", async () => { + const { cohortBank, unlockTime, owner } = await loadFixture(runEveryTime); + + await cohortBank.deposit({ value: TEST_AMOUNT }); + + const ownerBalanceBeforeWithdrawal = await ethers.provider.getBalance( + owner.address + ); + await time.increaseTo(unlockTime); + //withdraw + await cohortBank.withdraw(TEST_AMOUNT); + const ownerBalanceAfterWithdrawal = await ethers.provider.getBalance( + owner.address + ); + expect(ownerBalanceAfterWithdrawal).to.be.gt( + ownerBalanceBeforeWithdrawal + ); + }); + }); + runEveryTime(); + }); \ No newline at end of file From 98ddcdf7ddffa6bd2f43b27f4db56d121e79c352 Mon Sep 17 00:00:00 2001 From: Mac Date: Fri, 14 Apr 2023 22:12:18 -0500 Subject: [PATCH 3/3] chore: findings --- findings.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 findings.md diff --git a/findings.md b/findings.md new file mode 100644 index 0000000..66c6bbe --- /dev/null +++ b/findings.md @@ -0,0 +1,37 @@ +# + +# Findings of CohortBant Smart Contract + +CohortBant Smart Contract is a simple deposit and withdrawal contract witha an unlocktime, however as beautiful as this contract is, it is not without loopholes in it. + +# DEPOSIT FUNCTION + +The code block provided appears to be a simple function to allow users to deposit funds to the contract. However, there are a few potential vulnerabilities that can be identified: + +1. Reentrancy Attack: The code is not protected against reentrancy attacks. If an attacker were to call a function that executes before the require statement (e.g., a function that updates the sender's balance), the attacker could repeatedly call the deposit() function to withdraw more funds than they deposited. To mitigate this risk, you can add a reentrancy guard to the function using the "checks-effects-interactions" pattern. + +2. Lack of Access Control: The deposit() function is public, meaning anyone can call it and deposit funds to the contract. If the contract has other functions that require access control (e.g., only the contract owner can withdraw funds), this could be a vulnerability. To prevent unauthorized access, you can implement a role-based access control system. + +3.Lack of Input Validation: The require statement checks whether the msg.value is greater than 0, but it does not validate other inputs (e.g., whether the sender's address is valid). To prevent invalid inputs, you can add additional validation checks to the function. + +# WITHDRAWAL FUNCTION + +The vulnerability in the provided code is that the function withdraw() transfers the entire balance of the contract to the owner's address, regardless of the amount specified in the function argument 'amount'. This means that an attacker can exploit this vulnerability by passing in a large value for 'amount' and cause the entire balance of the contract to be transferred to the owner's address. + +To fix this vulnerability, the code should transfer only the specified 'amount' to the owner's address instead of transferring the entire balance. The modified code can look like this: + +function withdraw(uint256 amount) public { + +require(block.timestamp >= unlockTime, "You can't withdraw yet"); + +require(msg.sender == owner, "You aren't the owner"); + +require(amount <= address(this).balance, "Insufficient balance"); + +emit Withdrawal(amount, block.timestamp); + +payable(owner).transfer(amount); + +} + +Here, I have added a new require statement to check if the contract has sufficient balance to transfer the specified 'amount'. I have also modified the transfer statement to transfer only the specified 'amount' to the owner's address using the payable keyword. This ensures that the function withdraw() can only transfer the amount specified in the function argument and not the entire contract balance.