Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Missing Contract Owner role in the `RockPaperScissors` contract leading to an unguarded contract and role confusion

nebosvod

Description: When initially deployed, the RockPaperScissors sets the admin to be the contract deployer and doesn't keep track of the contract owner address anywhere. The owner function returns the adminAddress which is incorrect. There is also confusion in the setTimeout function which should be only callable by the admin but currently.

address public adminAddress;
​
constructor() {
winningToken = new WinningToken();
adminAddress = msg.sender;
}
​
function owner() public view returns (address) {
return adminAddress;
}
​
function setJoinTimeout(uint256 _newTimeout) external {
require(msg.sender == owner(), "Only owner can set timeout");
require(_newTimeout >= 1 hours, "Timeout must be at least 1 hour");
​
uint256 oldTimeout = joinTimeout;
joinTimeout = _newTimeout;
​
emit JoinTimeoutUpdated(oldTimeout, _newTimeout);
}

Impact: By not keeping the owner address anywhere in the contract, this leads to a contract that's solely owned by the admin. If thr admin is changed to be a different address, the initial contract owner will not have any ownership of the contract and therefore won't have any power over the contract. The contract deployer as a role will be nonexistent. This will lead to the admin having full ownership of the contract, the funds and setting new admins and timeouts.

Recommended Mitigation:
There are a different ways to mitigate this:

  1. Use the Ownable abstract class by openzeppelin by extending the RockPaperScissors contract which will make the initial deployer of the contract the owner and remove the owner function which incorrectly returns the admin instead. Also, change the require check in the setTimeout function to check that the msg.sender is the adminAddress instead of owner().

+ import "@openzeppelin/contracts/access/Ownable.sol";
+ contract RockPaperScissors is Ownable {
- constructor() {
+ constructor() Ownable (msg.sender) {
winningToken = new WinningToken();
adminAddress = msg.sender;
}
​
- function owner() public view returns (address) {
- return adminAddress;
- }
​
function setJoinTimeout(uint256 _newTimeout) external {
- require(msg.sender == owner(), "Only owner can set timeout");
- require(msg.sender == adminAddress, "Only owner can set timeout");
require(_newTimeout >= 1 hours, "Timeout must be at least 1 hour");
​
uint256 oldTimeout = joinTimeout;
joinTimeout = _newTimeout;
​
emit JoinTimeoutUpdated(oldTimeout, _newTimeout);
}
}

  1. Introduce a contractOwner address property and change the owner function to return this instead of the adminAddress.

+ address public owner;
​
constructor() {
winningToken = new WinningToken();
+ owner = msg.sender;
adminAddress = msg.sender;
}
​
function owner() public view returns (address) {
- return adminAddress;
+ return owner;
}
​
Updates

Appeal created

m3dython Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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