Summary
The setPassword function lacks the necessary access control mechanisms, creating a vulnerability that allows anyone to call it freely and change the password.
Vulnerability Details
function setPassword(string memory newPassword) external {
s_password = newPassword;
emit SetNetPassword();
}
The setPassword function is missing an onlyOwner modifier or a similar check with getPassword() function.
function getPassword() external view returns (string memory) {
@> if (msg.sender != s_owner) {
revert PasswordStore__NotOwner();
}
And here is the Foundry PoC:
function test_non_owner_can_set_password() public {
vm.startPrank(owner);
string memory ownerPasswordSet = "newOwnerPassword";
passwordStore.setPassword(ownerPasswordSet);
string memory ownerPasswordGet = passwordStore.getPassword();
vm.startPrank(address(1337));
string memory maliciousPasswordSet = "newMaliciousPassword";
passwordStore.setPassword(maliciousPasswordSet);
vm.startPrank(owner);
string memory finalPassword = passwordStore.getPassword();
assertNotEq(ownerPasswordGet, finalPassword);
assertEq(maliciousPasswordSet, finalPassword);
}
Impact
This is a critical security concern as it exposes the contract to unauthorized alterations of the stored password, jeopardizing data confidentiality and overall security.
Tools Used
Recommendations
There are three recommendations to be considered:
Implement onlyOwner modifier.
@@ -19,11 +19,22 @@ contract PasswordStore {
s_owner = msg.sender;
}
+ // https://solidity-by-example.org/function-modifier/
+ // Modifier to check that the caller is the owner of
+ // the contract.
+ modifier onlyOwner() {
+ require(msg.sender == s_owner, "Not owner");
+ // Underscore is a special character only used inside
+ // a function modifier and it tells Solidity to
+ // execute the rest of the code.
+ _;
+ }
+
/*
* @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 {
+ function setPassword(string memory newPassword) external onlyOwner {
s_password = newPassword;
emit SetNetPassword();
}
Add a similar check with revert like in getPassword().
@@ -24,6 +24,9 @@ contract PasswordStore {
* @param newPassword The new password to set.
*/
function setPassword(string memory newPassword) external {
+ if (msg.sender != s_owner) {
+ revert PasswordStore__NotOwner();
+ }
s_password = newPassword;
emit SetNetPassword();
}
Add a similar check like in getPassword() but with require.
@@ -24,6 +24,9 @@ contract PasswordStore {
* @param newPassword The new password to set.
*/
function setPassword(string memory newPassword) external {
+ require(msg.sender == s_owner, "Not owner");
s_password = newPassword;
emit SetNetPassword();
}
