First Flight #1: PasswordStore
First Flight #1
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Developer forgot to add the onlyOwner protection to setPassword()

aitor

Summary

The function comment for setPassword() reads: "This function allows only the owner to set a new password", but the developer
forgot to add some kind of modifier to secure the function, and it is external, so anyone can set the password at any time.

Vulnerability Details (PoC)

Check this test function created with Foundry

function test_setPasswordAsAttacker() external {
vm.prank(address(1));
passwordStore.setPassword("banana");
vm.prank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword, "banana");
}

If we execute it:

➜ 2023-10-PasswordStore git:(main) ✗ forge test --mt test_setPasswordAsAttacker
[⠢] Compiling...
[⠢] Compiling 1 files with 0.8.18
[⠆] Solc 0.8.18 finished in 902.07ms
Compiler run successful!
​
Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[PASS] test_setPasswordAsAttacker() (gas: 22234)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.31ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The user address(1) is able to set a new password.

Impact

Because any user can set and subsequently use a new password, any application or transaction that depends on it is compromised, as is any asset.

Tools Used

Foundry, manual.

Recommendations

The best way to secure an onlyOwner function is to use the Open Zeppelin library, as it has already been proven by high-level security researchers.
Check the information in: https://docs.openzeppelin.com/contracts/2.x/api/ownership#Ownable

As every function in the contract needs the onlyOwner protection, another way to secure the function is creating an onlyOwner modifier

modifier onlyOwner() {
if (msg.sender != s_owner) {
revert PasswordStore__NotOwner();
}
_;
}

We have to add it to the function

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

And run the same test to prove it works

2023-10-PasswordStore git:(main) ✗ forge test --mt test_setPasswordAsAttacker
[⠢] Compiling...
[⠒] Compiling 3 files with 0.8.18
[⠢] Solc 0.8.18 finished in 911.27ms
Compiler run successful!
​
Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[FAIL. Reason: PasswordStore__NotOwner()] test_setPasswordAsAttacker() (gas: 10771)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 426.38µs
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: PasswordStore__NotOwner()] test_setPasswordAsAttacker() (gas: 10771)
​
Encountered a total of 1 failing tests, 0 tests succeeded
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 2 years ago
inallhonesty Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-lacking-access-control

Anyone can call `setPassword` and set a new password contrary to the intended purpose.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.