Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

setPassword missing owner check allowing everyone to call it

Summary

The setPassword function should be callable by only the owner but check on the caller is missing.

Vulnerability Details

The function setPassword is external and is missing any check on the msg.sender, so anyone can call it.
Here is a Proof of Concept unit test demonstrating the issue (add it to PasswordStoreTest.t.sol):

function test_non_owner_can_set_password() public {
vm.prank(owner);
string memory ownerPassword = "ownerPassword";
passwordStore.setPassword(ownerPassword);
vm.prank(address(1));
string memory userPassword = "userPassword";
passwordStore.setPassword(userPassword);
vm.prank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword, userPassword);
}

Impact

Anyone can change the owner stored password overriding the previous one, updating it or deleting it.

Tools Used

Manual review.

Recommendations

You can implement in setPassword() the same check you have in getPassword().

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

Or better, now that you have the same code in two functions create a modifier with this check.

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

Now the two function signatures become:

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
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.