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

`setPassword` not checking if msg.sender is `s_owner` allows anyone to set a new password.

0xLuke4G1

Summary

The function PasswordStore::setPassword is intended to allow only the contract's owner to set a new password. However, due to a missing ownership check, any address can call this function and change the password.

Vulnerability Details

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

The function setPassword does not check if the caller is the contract's owner.

This allows any address to call the function and set a new password, violating the intended access control.

Checking the msg.sender and reverting the transaction if it doesn't correspond to s_owner will prevent this behavior.

Impact

The contract's owner does not have the exclusive right to set a new password since the function can be called by anyone.

Here's my proof of code:

function test_Not_only_owner_can_set_password() public {
// @dev 'owner' sets the password
string memory ownerPassword = "passwordOwner";
vm.prank(owner);
passwordStore.setPassword(ownerPassword);
​
address user = makeAddr("user");
// @dev an address 'user' tries to set a new password
string memory newUserPassword = "passwordUser";
vm.prank(user);
passwordStore.setPassword(newUserPassword);
​
// @dev get the actual password
vm.prank(owner);
string memory endingPassword = passwordStore.getPassword();
​
// @dev assert that the password has been changed
assert(keccak256(abi.encodePacked(endingPassword)) != keccak256(abi.encodePacked(ownerPassword)));
// @dev assert that the new password is the one set from the address 'user'
assert(keccak256(abi.encodePacked(endingPassword)) == keccak256(abi.encodePacked(newUserPassword)));
}
​
function testFuzz_Everybody_can_set_password(address user) public {
// @dev 'owner' sets the password
string memory ownerPassword = "passwordOwner";
vm.prank(owner);
passwordStore.setPassword(ownerPassword);
​
// @dev an address 'user' tries to set a new password
string memory newUserPassword = "passwordUser";
vm.prank(user);
passwordStore.setPassword(newUserPassword);
​
// @dev get the actual password
vm.prank(owner);
string memory endingPassword = passwordStore.getPassword();
​
// @dev assert that the password has been changed
assert(keccak256(abi.encodePacked(endingPassword)) != keccak256(abi.encodePacked(ownerPassword)));
// @dev assert that the new password is the one set from the address 'user'
assert(keccak256(abi.encodePacked(endingPassword)) == keccak256(abi.encodePacked(newUserPassword)));
}

Tools Used

  • foundry

Recommendations

Add a check to setPassword that reverts if the msg.sender is not s_owner.

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

To facilitate secure ownership operations, I strongly recommend implementing the Ownable contract from the OpenZeppelin library.

This contract provides a simple and reliable way to handle ownership of the contract, including an onlyOwner modifier that can be used to restrict functions only to the contract's owner.

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.