Find The Bug in this Smart Contract [series] 2024

// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;

/*
 * @author not-so-secure-dev
 * @title PasswordStore
 * @notice This contract allows you to store a private password that others won't be able to see. 
 * You can update your password at any time.
 */
contract PasswordStore {
    error PasswordStore__NotOwner();

    address private s_owner;
    //this is not really private 
    //all data onchain is public infomation
    string private s_password;

    event SetNetPassword();

    constructor() {
        s_owner = msg.sender;
    }

    /*
     * @notice This function allows only the owner to set a new password.
     * @param newPassword The new password to set.
     */

    //is it an external call 
    //if it is only the owner can call it
    //q why is the constructor owner not set in the contract?
    //anybody can set the owner to be themselves
    //missing access control

    function setPassword(string memory newPassword) external {
        s_password = newPassword;
        emit SetNetPassword();
    }

    /*
     * @notice This allows only the owner to retrieve the password.
     * @param newPassword The new password to set.
     */
    function getPassword() external view returns (string memory) {
        if (msg.sender != s_owner) {
            revert PasswordStore__NotOwner();
        }
        return s_password;
    }
}

This is a smart contract I worked on, and I found three bugs in it. This tutorial. I shared how I could find the bugs, the mitigated response to the bugs, and the best way to prevent bugs. 

Smart contracts take a lot of time to edit and find bugs, mostly when it has a lot of lines and inherit from other contracts, it is hard to audit and fish out leaks. 

I wrote Top 9 Hacks in web3, where I shared some of the hacks and how web3 is becoming better with audits like this. 

Before diving into the audit, you must pay attention to the following to understand what the client needs and how best to prepare yourself. 

Scope the files - Understand what the clients want, and ensure you are working on the actual version so you don't waste your time only to be told that's not what you should be looking at. 

Ask Questions - Do not be scared to ask questions. The smart contract developers are best positioned to explain why certain things are how they are, “DO NOT ASSUME”. 

Read the Documentation- Start from the documentation to understand what the contract hopes to achieve. Read through the step-by-step analysis this would give you a deeper understanding of what should be in the codes and what should not. 

Document your process from the beginning, and you can use a markdown file, comments, notes, or whiteboard, use whatever to keep track of where you are so you don't get lost, as some codes are quite lengthy. 

Armed with this step, let us look at the code above and find some bugs and how we can protect this contract from such bugs.

[S-#] Storing the password on-chain makes it visible to anyone and no longer private

Description: All data stored on the chain is visible to anyone even when it explicitly says only the PasswordStore::s_owner is the only one that can set a password.

The PasswordStore::getpassword function does not limit who sets passwords to the owner of the contracts.

See below for proof of concept.

Impact: Anyone can read the private password, severely breaking the functionality of the protocol

Proof of Concept: - First i set my forge init --force to update my foundry setup

• spin up my local chain

make anvil

• to run the contract

make deploy

• this will get the hash of the password stored in

cast storage [address of the contract][contract PasswordStore 0x5FbDB2315678afecb367f032d93F642f64180aa3] --rpc-url http//127.0.0.1:8545

• pass word stored here will be in the hash

string private s_password;

• it will produce the password of the owner

cast parse-bytes32-string [hash value]

You will get the password on the chain.

Recommended Mitigation: I believe passwords should not be stored this way, as it will leave chances to anyone picking through the password; passwords should be stored out of the chain, and decrypt hash should be implemented to encrypt and then decrypt using password hash.

[S-#] No access control: Anyone can call and change the password

Description: The PasswordStore::setPassword function is set to be an external function; however, the entire focus of the contract is that his function allows only the owner to set a new password.

function setPassword(string memory newPassword) external {
    s_password = newPassword;
        emit SetNetPassword();
    }

Impact: Anyone can change the password thereby severely limiting the value of the contract's intended usage

Proof of Concept: Add the following to the Password.t.sol test file

code

function test_anyone_can_set_password() public {
        string memory expectedPassword = "anyonesPassword";

        // Set the password as a non-owner
        vm.startPrank(address(1));
        passwordStore.setPassword(expectedPassword);
        string memory actualPassword = passwordStore.getPassword();
        assertEq(actualPassword, expectedPassword);
    }

Recommended Mitigation: Add access control to setPassword function

if(msg.sender != s_owner){
    revert Passsword__Not_Owner()
}

[S-#] Parameter does not exist but included

Description: The PasswordStore::getPassowrd function signature is getPassword while the natspec says it should include a string a parameter getPassowrd(string).

Impact: Wrong, misleading concept

Recommended Mitigation: Remove the line or correct it to meet the code structure

- * @param newPassword The new password to set.