Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Estheroche cohort bank testing #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Estheroche cohort bank testing #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
321cc52
feat: create CohortBank contract
sprtd Apr 10, 2023
5831349
test/cohortBank
Apr 14, 2023
98ddcdf
chore: findings
Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
51 changes: 51 additions & 0 deletions contracts/CohortBank.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
37 changes: 37 additions & 0 deletions findings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
252 changes: 126 additions & 126 deletions test/Lock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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]
// );
// });
// });
// });
// });
Loading