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

`private` does not mean hidden, `s_password` visible to anyone

stakog

Summary

Using a private variable is not the way to hide secret information, since it is still stored on the blockchain visible to anyone.

Vulnerability Details

Adding the private specifier to a state variable does not mean that the variable is hidden from the world. Therefore, it is not a proper way to store a password.
A private variable sets the visibility of that variable for contracts, while users can still read its value from the blockchain.
A private variable just means that it can only be read or modified from its own contract (in contrast to derived or external contracts).
Meanwhile, the private state variable is still located on the blockchain in a storage memory slot which can be read by anyone, so the password is not hidden.
This is clearly stated in the solidity docs: https://docs.soliditylang.org/en/v0.8.21/contracts.html#state-variable-visibility.

Impact

The password is still visible to anyone in the world. This is in contrast to what the contract was supposed to do, as can be understood from the comment @notice This contract allows you to store a private password that others won't be able to see.

We don't really know what the password is used for, but passwords are usually used to protect sensitive information or funds. Therefore, a compromised password very often means a lot of damage. Also, as can be seen in the test code provided below, it is pretty easy to read s_password from storage. This is why the severity for this bug is high.

Here is a test function with comments, to show that after the owner sets the password, an attacker can read it from storage:

function test_password_visible_to_anyone() public {
​
// set a password as owner
vm.startPrank(owner);
string memory expectedPassword = "ownerPassword";
passwordStore.setPassword(expectedPassword);
vm.stopPrank();
​
bytes32 loadedPasswordBytes32 = vm.load(address(passwordStore),bytes32(uint256(1))); // read slot 1 (slot 0 is owner's address)
bytes32 temp = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00;
loadedPasswordBytes32 = loadedPasswordBytes32 & temp; // This is to eliminate the right-most byte, which represents the length of the data in the slot.
​
bytes32 expectedPasswordBytes32 = bytes32(abi.encodePacked(expectedPassword)); // convert string to bytes32
​
// Assert that the password we read is the password that was set
assertEq(loadedPasswordBytes32, expectedPasswordBytes32);
​
}

Tools Used

  • Foundry

Recommendations

Storing passwords, or any other secret information, is problematic on a blockchain since everything is visible.
We don't know what the password is used for, but a fair assumption is that it is used for authentication of users. On a blockchain, it is far batter to authenticate using the caller's address in the msg.sender global variable.
This recommendation effectively renders PasswordStore contract useless.

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-anyone-can-read-storage

Private functions and state variables are only visible for the contract they are defined in and not in derived contracts. In this case private doesn't mean secret/confidential

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.