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..418f2b5 --- /dev/null +++ b/contracts/CohortBank.sol @@ -0,0 +1,51 @@ +// 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); + } + + 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(amount, block.timestamp); + + owner.transfer(address(this).balance); + } +} 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. 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