First Flight #1: PasswordStore

Summary

The password can be revealed by decoding the transaction calldata

Vulnerability Details

All data and transaction details on a blockchain are stored on a public ledger. This means that when function setPassword() is called, the password value set during the call is visible to anyone who queries the blockchain and decodes the calldata.

Even if there is access control on the getPassword() function, an attacker can still obtain the password from the transaction calldata details used to transact with the contract.

##PoC

On the example below we can decode the password from the calldata

when the function is called, like this setPassword(mysecret123), the calldata that is sent with the transaction we can see it in foundry by running this command

`
cast calldata "setPassword(string memory)" mysecret123
0x290bb4530000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000b6d79736563726574313233000000000000000000000000000000000000000000

`
This is the calldata that an attacker can get from the blockchain and decode it to get the password.
in foundry we can run:

cast calldata-decode "setPassword(string memory)" 0x290bb453000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000 0000b6d79736563726574313233000000000000000000000000000000000000000000 mysecret123

and there we see the password is displayed. The attacker can then use the password to access other parts of the system

Impact

Tools Used

Manual code review and Foundry

Recommendations

We need to store the hash of the password, not the password itself. The password should be hashed using a one-way cryptographic function like keccak256. This way, even if an attacker decodes the calldata, they will only have access to the hash, not the actual password. The password should be hashed off-chain before the setPassword() function is called. The resulting hash is then stored in the contract’s state on the blockchain. Given this hash, it’s not possible to determine the original password. During authentication, the entered password should be hashed in the same way, and this hash should be compared to the stored hash for verification

here is the modified setPassword and getPassword functions that now takes bytes and not string

`
function setPassword(bytes32 newPasswordHash) external onlyOwner() {
s_passwordHash = newPasswordHash;
emit SetNetPassword();
}

function getPassword() external view onlyOwner() returns (bytes32) {
return s_passwordHash;
`

here are the tests that first hash the password:
`
function testOwnerCanSetPassword() public {

    vm.startPrank(owner);

    bytes32 expectedPassword = keccak256(abi.encodePacked("myNewPassword")) ;
    passwordStore.setPassword(expectedPassword);
    bytes32 actualPassword = passwordStore.getPassword();
    assertEq(actualPassword, expectedPassword);
}

function testNonOwnerReadingPasswordReverts() public {
    vm.startPrank(address(1));

    vm.expectRevert(PasswordStore.PasswordStore__NotOwner.selector);
    passwordStore.getPassword();
}

function testFailNonOwnerCanSetPassword() public {
    
    vm.startPrank(address(1));
   
    bytes32 expectedPassword = keccak256(abi.encodePacked("myNewPassword"));
    passwordStore.setPassword(expectedPassword);
    
              
}

Now, when we run the tests below, we no longer see the plaintext password as before. If an attacker decodes the calldata, they will only obtain the hash, not the actual password. This is because we have hashed the password using a one-way cryptographic hash function. Therefore, it’s impossible for them to compute the original password from the hash
forge test -vvvv
[⠔] Compiling...
No files changed, compilation skipped

Running 3 tests for test/PasswordStore.t.sol:PasswordStoreTest
[PASS] testFailNonOwnerCanSetPassword() (gas: 10671)
Traces:
[10671] PasswordStoreTest::testFailNonOwnerCanSetPassword()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [2434] PasswordStore::setPassword(0x21f88f2924c62b1c200a1ddee240efad40a810a0432f05f460152806adf1ab0a)
│ └─ ← "Caller is not the owner"
└─ ← "Caller is not the owner"

[PASS] testNonOwnerReadingPasswordReverts() (gas: 10732)
Traces:
[10732] PasswordStoreTest::testNonOwnerReadingPasswordReverts()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [0] VM::expectRevert(PasswordStore__NotOwner())
│ └─ ← ()
├─ [2328] PasswordStore::getPassword() [staticcall]
│ └─ ← "PasswordStore__NotOwner()"
└─ ← ()

[PASS] testOwnerCanSetPassword() (gas: 19427)
Traces:
[19427] PasswordStoreTest::testOwnerCanSetPassword()
├─ [0] VM::startPrank(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
│ └─ ← ()
├─ [8118] PasswordStore::setPassword(0x21f88f2924c62b1c200a1ddee240efad40a810a0432f05f460152806adf1ab0a)
│ ├─ emit SetNetPassword()
│ └─ ← ()
├─ [440] PasswordStore::getPassword() [staticcall]
│ └─ ← 0x21f88f2924c62b1c200a1ddee240efad40a810a0432f05f460152806adf1ab0a
└─ ← ()

`