First Flight #1: PasswordStore

Summary

According to the function comments,
the setPassword() function in PasswordStore.sol should allow only the owner to set a new password. If this is the case then this function lacks necessary access control, allowing any address to call it and effectively set the password.

Vulnerability Details

The setPassword() function is intended to allow only the owner to set a new password. However, currently, the password set by the owner can be reset by anyone because the function lacks access control that restricts access to the owner. This allows an attacker to easily set their own password and use it in any other functionality that might rely on the password set in this contract.

##Proof of Concept
Add a new failing test function testFailNonOwnerCanSetPassword() this function passes if it reverts when it's called by an address who is not the owner of the contract.

`
function testFailNonOwnerCanSetPassword() public {

    vm.startPrank(address(1));
   
    string memory expectedPassword = "PasswordHack";
    passwordStore.setPassword(expectedPassword);

}
`

running this test with --vvvvv
`
~/2023-10-PasswordStore$ forge test -vvvvv --match-test testFailNonOwnerCanSetPassword
[⠔] Compiling...
[⠘] Compiling 1 files with 0.8.18
[⠃] Solc 0.8.18 finished in 1.18s
Compiler run successful!

Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[FAIL. Reason: Assertion failed.] testFailNonOwnerCanSetPassword() (gas: 15129)
Traces:
[741837] PasswordStoreTest::setUp()
├─ [356698] → new DeployPasswordStore@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← 1671 bytes of code
├─ [285684] DeployPasswordStore::run()
│ ├─ [0] VM::startBroadcast()
│ │ └─ ← ()
│ ├─ [225780] → new PasswordStore@0x90193C961A926261B756D1E5bb255e67ff9498A1
│ │ └─ ← 1017 bytes of code
│ ├─ [23786] PasswordStore::setPassword(myPassword)
│ │ ├─ emit SetNetPassword()
│ │ └─ ← ()
│ ├─ [0] VM::stopBroadcast()
│ │ └─ ← ()
│ └─ ← PasswordStore: [0x90193C961A926261B756D1E5bb255e67ff9498A1]
└─ ← ()

[15129] PasswordStoreTest::testFailNonOwnerCanSetPassword()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [6686] PasswordStore::setPassword(PasswordHack)
│ ├─ emit SetNetPassword()
│ └─ ← ()
└─ ← ()

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.94ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/PasswordStore.t.sol:PasswordStoreTest
[FAIL. Reason: Assertion failed.] testFailNonOwnerCanSetPassword() (gas: 15129)

Encountered a total of 1 failing tests, 0 tests succeeded
`

The test failed because the function did not revert. the logs show that the SetNewPassword() event was emitted meaning a non owner has successfully set the password.

Impact

The lack of access control on the setPassword() function, could lead to unauthorized access and misuse of the contract by an attacker who could potentially set their own password. Moreover, it poses a significant security risk, especially if other functionalities of the system rely on the password set in this contract, potentially compromising the entire system.

Tools Used

Manual code review

Recommendations

To ensure that only the owner can call the setPassword function, add a modifier that checks if the caller is the owner.
Below the constructor add the modifier code:
modifier onlyOwner() { require(msg.sender == s_owner, "Caller is not the owner"); _; }

use the modifier on function setPassword()

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

With this modification, only the owner address that deployed the contract can call setPassword. If any other address tries to call it, an error will be thrown. This ensures that only the owner can change or set the password.

rerunning the test we see that failing test is now passing meaning an address that is not the owner address cannot set call setPassword() anymore:

`
~/2023-10-PasswordStore$ forge test -vvvvv --match-test testFailNonOwnerCanSetPassword
[⠔] Compiling...
[⠘] Compiling 3 files with 0.8.18
[⠃] Solc 0.8.18 finished in 1.19s
Compiler run successful!

Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[PASS] testFailNonOwnerCanSetPassword() (gas: 11205)
Traces:
[779652] PasswordStoreTest::setUp()
├─ [375517] → new DeployPasswordStore@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← 1765 bytes of code
├─ [304661] DeployPasswordStore::run()
│ ├─ [0] VM::startBroadcast()
│ │ └─ ← ()
│ ├─ [244598] → new PasswordStore@0x90193C961A926261B756D1E5bb255e67ff9498A1
│ │ └─ ← 1111 bytes of code
│ ├─ [23926] PasswordStore::setPassword(myPassword)
│ │ ├─ emit SetNetPassword()
│ │ └─ ← ()
│ ├─ [0] VM::stopBroadcast()
│ │ └─ ← ()
│ └─ ← PasswordStore: [0x90193C961A926261B756D1E5bb255e67ff9498A1]
└─ ← ()

[11205] PasswordStoreTest::testFailNonOwnerCanSetPassword()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [2754] PasswordStore::setPassword(PasswordHack)
│ └─ ← "Caller is not the owner"
└─ ← "Caller is not the owner"

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.91ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
`