First Flight #1: PasswordStore

Summary

An attacker can change the password set by the password owner to a new one.

Vulnerability Details

function setPassword(string memory newPassword) external {
s_password = newPassword;
emit SetNetPassword();
}
https://github.com/Cyfrin/2023-10-PasswordStore/blob/856ed94bfcf1031bf9d13514cb21b591d88ed323/src/PasswordStore.sol#L26-L29

The setPassword function is vulnerable because of two things. One, the visibility of the function is set to external. By this, anyone can call the function. Two, there is no access control in place that restricts who can call the function.

Anyone can call the function and set a new password.

Here's a foundry test showing this vulnerability, particularly the testNonOwnerCanSetPassword():

Note: The test uses the exact environment setup provided for the contest - with the addition of the testNonOwnerCanSetPassword().

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

import {Test, console} from "forge-std/Test.sol";
import {PasswordStore} from "../src/PasswordStore.sol";
import {DeployPasswordStore} from "../script/DeployPasswordStore.s.sol";

contract PasswordStoreTest is Test {
PasswordStore public passwordStore;
DeployPasswordStore public deployer;
address public owner;
address anyUser = address(2);

function setUp() public {
    deployer = new DeployPasswordStore();
    passwordStore = deployer.run();
    owner = msg.sender;
}

function test_owner_can_set_password() public {
    vm.startPrank(owner);
    string memory expectedPassword = "myNewPassword";
    passwordStore.setPassword(expectedPassword);
    string memory actualPassword = passwordStore.getPassword();
    assertEq(actualPassword, expectedPassword);
}

function testNonOwnerCanSetPassword() public {
    // Switch to non-owner account
    vm.startPrank(address(2));
    string memory expectedPassword = "newPassword";

    // Set new password as non-owner
    passwordStore.setPassword(expectedPassword);
    vm.stopPrank();

    // Switch back to owner account since only "owner" can call "getPassword()"
    vm.startPrank(owner);

    // Owner retrieves password
    string memory actualPassword = passwordStore.getPassword();

    // Assert actual password matches what non-owner set
    assertEq(actualPassword, expectedPassword);
}

function test_non_owner_reading_password_reverts() public {
    vm.startPrank(address(1));
    vm.expectRevert(PasswordStore.PasswordStore__NotOwner.selector);
    passwordStore.getPassword();
}

}

Impact

An attacker can change the password set by the password owner to a new one.

Tools Used

Manual review

Recommendations

Access to the setPassword function should be restricted to only the owner. Like so:

if (msg.sender != s_owner) revert "Not owner";