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

`setPassword` function does not restrict any user to override the owner password.

alymurtazamemon

Summary

The requirement of this code is that only the owner may set and retrieve their password. Unfortunately, this is not the case with the setPassword function, which does not contain an access modifier to prevent other users from overriding the owner's password.

Vulnerability Details

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

PasswordStore.sol - Lines 26 - 29

This function is open for everyone who can override the s_password state variable.

Impact

Although this allows malicious users to override the s_password state variable, they cannot actually see the real password set by the owner. However, the owner will need to reset the password every time, incurring gas costs, or else the password will be exposed.

Due to that, I am selecting medium for this issue.

POC

function test_othersCanOverridePassword() public {
vm.startPrank(address(1));
string memory expectedPassword = "maliciousPassword";
passwordStore.setPassword(expectedPassword);
vm.stopPrank();
​
vm.startPrank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword, expectedPassword);
}

Add this test to PasswordStore.t.sol file and run it using command forge test --match-test test_othersCanOverridePassword -vvvv

OUTPUT

alymurtazamemon@Alis-MacBook-Air 2023-10-PasswordStore % forge test --match-test test_othersCanOverridePassword -vvvv
[⠢] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[PASS] test_othersCanOverridePassword() (gas: 22914)
Traces:
[22914] PasswordStoreTest::test_othersCanOverridePassword()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [6686] PasswordStore::setPassword(maliciousPassword)
│ ├─ emit SetNetPassword()
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
│ └─ ← ()
├─ [3320] PasswordStore::getPassword() [staticcall]
│ └─ ← maliciousPassword
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 545.79µs
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommendations

Convert this code to modifier and use it on both functions.

function getPassword() external view returns (string memory) {
- if (msg.sender != s_owner) {
- revert PasswordStore__NotOwner();
- }
​
+ modifier onlyOwner() {
+ if (msg.sender != s_owner) {
+ revert PasswordStore__NotOwner();
+ }
+ _;
+ }
​
+ function setPassword(string memory newPassword) external onlyOwner {
+ function getPassword() external view onlyOwner returns (string memory) {
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.

Give us feedback!