Rock Paper Scissors

Summary

The RockPaperScissors contract repeats the require(msg.sender == adminAddress, ...) check in multiple functions (setAdminAddress, withdrawTokens, setGameTimeout), reducing code maintainability. A custom modifier would centralize this logic.

Vulnerability Details

The RockPaperScissors contract uses the require(msg.sender == adminAddress, "Only admin can set new admin") check (or similar variations) in at least three functions: setAdminAddress, withdrawTokens, and setGameTimeout. This repetition of admin access control logic increases the risk of inconsistency if the admin address or error message needs to change. For example, updating the admin check would require modifying each instance individually, which is error-prone and violates the DRY (Don’t Repeat Yourself) principle. Centralizing the logic in a modifier would improve readability and maintainability.

Impact

• Reduced code readability due to repeated admin check logic across multiple functions.

• Increased maintenance burden, as changes to the admin address or error message require updating at least three function instances, risking inconsistencies.

• Potential for bugs if one instance is overlooked during updates (e.g., using a different error message or condition).

• No security impact, but violates best practices for clean, maintainable smart contract code, justifying low severity.

Tools Used

Manual Code Review

Recommendations

Introduce a custom onlyAdmin modifier to centralize the admin access check, replacing repeated require(msg.sender == adminAddress, ...) statements. Apply the modifier to all admin-only functions to improve readability and maintainability.