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.

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

Onboarded #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 6 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@

A smart contract applicatoin for storing a password. Users should be able to store a password and then retrieve it later. Others should not be able to access the password.

[Testnet deployment](https://sepolia.etherscan.io/address/0x2ecf6ad327776bf966893c96efb24c9747f6694b)

- [PasswordStore](#passwordstore)
- [Getting Started](#getting-started)
- [Requirements](#requirements)
- [Quickstart](#quickstart)
- [Optional Gitpod](#optional-gitpod)
- [Usage](#usage)
- [Deploy (local)](#deploy-local)
- [Testing](#testing)
- [Test Coverage](#test-coverage)
- [Audit Scope Details](#audit-scope-details)
- [Create the audit report](#create-the-audit-report)
- [Roles](#roles)

# Getting Started

Expand All @@ -38,12 +39,6 @@ cd 3-passwordstore-audit
forge build
```

### Optional Gitpod

If you can't or don't want to run and install locally, you can work with this repo in Gitpod. If you do this, you can skip the `clone this repo` part.

[![Open in Gitpod](https://gitpod.io/button/open-in-gitpod.svg)](https://gitpod.io/#github.com/Cyfrin/3-passwordstore-audit)

# Usage

## Deploy (local)
Expand Down Expand Up @@ -91,11 +86,8 @@ forge coverage --report debug
- Solc Version: 0.8.18
- Chain(s) to deploy contract to: Ethereum

## Create the audit report
## Roles

View the [audit-report-templating](https://github.com/Cyfrin/audit-report-templating) repo to install all dependencies.
- Owner: The user who can set the password and read the password.
- Outsides: No one else should be able to set or read the password.

```bash
cd audits
pandoc 2023-09-01-password-store-report.md -o report.pdf --from markdown --template=eisvogel --listings
```
11 changes: 11 additions & 0 deletions audit/finding1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
### [S-#] TITLE (Root Cause + Impact)

**Description:** All data stored on chain is visible to anyone and can be read directly by blockchain .
The `PasswordStore::s_password` variable is intended to be private and can only be called by owner

**Impact:** Anyone can read the password and set password.

**Proof of Concept:** (proof of code) The below test case shows how anyone can read the password directly from blockchain

**Recommended Mitigation:**
Dont add the view function and also the encrypt the password on chain and store it in off chain.
34 changes: 34 additions & 0 deletions audit/finding2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
### [S-#] `PasswordStore::setPassword` has no access control , meaning a non owner could change it

**Description:** The `PasswordStore::setPassword` function set_password is set to `external` function, however the function and overall purpose of smartcontract is that `only owners are allowed to set password`

**Impact:** anyone can set change the password of contract

**Proof of Concept:**
Add the following to t.sol file
<details>

```javascript
function test_anyone_can_set_password(address randomAddress) public{
vm.assume(randomAddress!=owner);
vm.prank(randomAddress);
string memory expectedPassword = "myNewPassword";
passwordStore.setPassword(expectedPassword);
vm.prank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword,expectedPassword);
}


```

</details>

**Recommended Mitigation:** Add an access control conditional to `setPassword` function

```javascript
if (!msg.sender != s_owner){
revert Password_NotOwner();
}

```
15 changes: 15 additions & 0 deletions audit/finding3.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
### [S-3] `PasswordStore::getPassword` has a parameter doesnt exist

**Description:** The `PasswordStore::getPassword` function set_password is set to `external` function, however the function and overall purpose of smartcontract is that `only owners are allowed to get password`

**Impact:** natspec




**Recommended Mitigation:** remove natspec

```diff
- * @param newPassword The new password to set.

```
62 changes: 62 additions & 0 deletions audit/findings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
### [S-1] ONCHAIN STATE VARIABLE

**Description:** All data stored on chain is visible to anyone and can be read directly by blockchain .
The `PasswordStore::s_password` variable is intended to be private and can only be called by owner

**Impact:** Anyone can read the password and set password.

**Proof of Concept:** (proof of code) The below test case shows how anyone can read the password directly from blockchain

**Recommended Mitigation:**
Dont add the view function and also the encrypt the password on chain and store it in off chain.

### [S-2] `PasswordStore::setPassword` has no access control , meaning a non owner could change it

**Description:** The `PasswordStore::setPassword` function set_password is set to `external` function, however the function and overall purpose of smartcontract is that `only owners are allowed to set password`

**Impact:** anyone can set change the password of contract

**Proof of Concept:**
Add the following to t.sol file
<details>

```javascript
function test_anyone_can_set_password(address randomAddress) public{
vm.assume(randomAddress!=owner);
vm.prank(randomAddress);
string memory expectedPassword = "myNewPassword";
passwordStore.setPassword(expectedPassword);
vm.prank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword,expectedPassword);
}


```

</details>

**Recommended Mitigation:** Add an access control conditional to `setPassword` function

```javascript
if (!msg.sender != s_owner){
revert Password_NotOwner();
}

```

### [S-3] `PasswordStore::getPassword` has a parameter doesnt exist

**Description:** The `PasswordStore::getPassword` function set_password is set to `external` function, however the function and overall purpose of smartcontract is that `only owners are allowed to get password`

**Impact:** natspec




**Recommended Mitigation:** remove natspec

```diff
- * @param newPassword The new password to set.

```
85 changes: 85 additions & 0 deletions minimal-onboarding-filled.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# Minimal Smart Contract Security Review Onboarding

# Table of Contents

- [Minimal Smart Contract Security Review Onboarding](#minimal-smart-contract-security-review-onboarding)
- [Table of Contents](#table-of-contents)
- [About the project / Documentation](#about-the-project--documentation)
- [Stats](#stats)
- [Setup](#setup)
- [Requirements](#requirements)
- [Quickstart](#quickstart)
- [Testing](#testing)
- [Security Review Scope](#security-review-scope)
- [Commit Hash](#commit-hash)
- [Repo URL](#repo-url)
- [In scope vs out of scope contracts](#in-scope-vs-out-of-scope-contracts)
- [Compatibilities](#compatibilities)
- [Roles](#roles)
- [Known Issues](#known-issues)

# About the project / Documentation

A smart contract applicatoin for storing a password. Users should be able to store a password and then retrieve it later. Others should not be able to access the password.

# Stats

- nSLOC: 20
- Complexity Score: 10
- Security Review Timeline: Date -> Date

# Setup

## Requirements

- [git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git)
- You'll know you did it right if you can run `git --version` and you see a response like `git version x.x.x`
- [foundry](https://getfoundry.sh/)
- You'll know you did it right if you can run `forge --version` and you see a response like `forge 0.2.0 (816e00b 2023-03-16T00:05:26.396218Z)`

## Quickstart

```
git clone https://github.com/Cyfrin/3-passwordstore-audit
cd 3-passwordstore-audit
forge build
```

## Testing

```
forge test
```

# Security Review Scope

## Commit Hash

7d55682ddc4301a7b13ae9413095feffd9924566

## Repo URL

https://github.com/Cyfrin/3-passwordstore-audit

## In scope vs out of scope contracts

```
./src/
└── PasswordStore.sol
```

## Compatibilities

- Solc Version: 0.8.18
- Chain(s) to deploy contract to:
- ETH
- Tokens: None

# Roles

- Owner: The user who can set the password and read the password.
- Outsides: No one else should be able to set or read the password.

# Known Issues

None
92 changes: 92 additions & 0 deletions minimal-onboarding-questions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Minimal Smart Contract Security Review Onboarding

# Table of Contents

- [Minimal Smart Contract Security Review Onboarding](#minimal-smart-contract-security-review-onboarding)
- [Table of Contents](#table-of-contents)
- [About the project / Documentation](#about-the-project--documentation)
- [Stats](#stats)
- [Setup](#setup)
- [Requirements](#requirements)
- [Testing](#testing)
- [Security Review Scope](#security-review-scope)
- [Commit Hash](#commit-hash)
- [Repo URL](#repo-url)
- [In scope vs out of scope contracts](#in-scope-vs-out-of-scope-contracts)
- [Compatibilities](#compatibilities)
- [Roles](#roles)
- [Known Issues](#known-issues)

# About the project / Documentation

*Summary of the project. The more documentation, the better.*

# Stats

*Use something like solidity metrics or cloc to get these.*

- nSLOC: XX
- Complexity Score: XX
- Security Review Timeline: Date -> Date

# Setup

## Requirements

*What tools are needed to setup the codebase & test suite?*

Example:
```bash
forge init
forge install OpenZeppelin/openzeppelin-contracts --no-commit
forge install vectorized/solady --no-commit
forge build
```

## Testing

*How to run tests. How to see test coverage.*

Example:
```bash
forge test
```

# Security Review Scope

*The specific details of the security review. Nail down exactly what the protocol is planning on deploying, and how they plan on deploying it.*

## Commit Hash
## Repo URL
## In scope vs out of scope contracts
## Compatibilities

- Solc Version: XXX
- Chain(s) to deploy contract to:
- XXX (ie: ETH)
- XXX (ie: Arbitrum)
- Tokens:
- XXX (ie: ERC20s)
- XXX (ie: LINK: <address>)
- XXX (ie: USDC: <address>)
- XXX (ie: ERC721s)
- XXX (ie: CryptoKitties: <address>)
- *List expected ERC20s and other specific tokens. If a protocol is expected to work with multiple or any tokens of a certain standard, you could do something like "All ERC20s". Or an ordered list like "USDC: <USDC Address>" etc*

# Roles

*What are the different actors of the system? What are their powers? What should/shouldn't they do?*

Example:
```
Actors:
Buyer: The purchaser of services, in this scenario, a project purchasing an audit.
Seller: The seller of services, in this scenario, an auditor willing to audit a project.
Arbiter: An impartial, trusted actor who can resolve disputes between the Buyer and Seller.
The Arbiter is only compensated the arbiterFee amount if a dispute occurs.
```

# Known Issues

*List any issues that the protocol team is aware of and will not be acknowledging/fixing.*
4 changes: 4 additions & 0 deletions notes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#about the project

A user is allowed to set a password but cant see it

Loading