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

Everyone can call the `setPassword()` function

bube

Summary

The function setPassword() in contract PasswordStore.sol can be called by any user, not only by owner. That means, everyone can set new password to the owner.

Vulnerability Details

The setPassword() function currently lacks appropriate access control measures. Despite the @notice indicating that only the owner can set a new password, there is no verification process in place to enforce this. The function setPassword() is external and can be called by any user, allowing them to alter the password. This represents a significant security vulnerability that needs to be fixed.

@> * @notice This function allows only the owner to set a new password.
* @param newPassword The new password to set.
*/
@> function setPassword(string memory newPassword) external {
//@audit The function is external and there are no restrictions of that who can call this function and change the password!
@> s_password = newPassword;
emit SetNetPassword();
}

Impact

The function setPassword() is external and it doesn't revert when executed by non-owner, thereby allowing unauthorized users to change the password.
This security flaw is demonstrated in the test function test_non_owner_can_set_password_reverts() which shows that setPassword() doesn't revert when a non-owner executes it. This test function can be added to the PasswordStore.t.sol file and executed in Foundry using the following command: forge test --match-test test_non_owner_can_set_password_reverts -vvv

function test_non_owner_can_set_password_reverts() public {
vm.startPrank(address(1));
string memory expectedPassword = "myNewPassword";
vm.expectRevert(PasswordStore.PasswordStore__NotOwner.selector);
passwordStore.setPassword(expectedPassword);
}

After executing the test, the result is:

Traces:
[15419] PasswordStoreTest::test_non_owner_can_set_password_reverts()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [0] VM::expectRevert(PasswordStore__NotOwner())
│ └─ ← ()
├─ [6686] PasswordStore::setPassword(myNewPassword)
│ ├─ emit SetNetPassword()
│ └─ ← ()
└─ ← "Call did not revert as expected"
​
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.10ms

Also, there is no test case in PasswordStore.t.sol where a non-owner calls the function setPassword().

Tools Used

VS Code, Foundry

Recommendations

In getPassword() function there is a statement that checks if the function is invoked by the owner and it reverts if not. Add a similar verification statement in setPassword() function to enhance security.

function setPassword(string memory newPassword) external {
+ if (msg.sender != s_owner) {
+ revert PasswordStore__NotOwner();
+ }
s_password = newPassword;
emit SetNetPassword();
}

Also, you can use onlyOwner modifier from the OpenZeppelin's Ownable contract to restrict access to the setPassword and getPassword functions to the owner of the contract. The owner is set to the address that deploys the contract, and can be transferred to another address using the transferOwnership() function provided by the Ownable contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge
almost 2 years ago
inallhonesty Lead Judge almost 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.