weird behaviour of `restricted` modifier

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x871fe8a1fba8836c1f942d2bde36cb76b3e633dddf18fbc3bd8c3c43e296277a Severity: minor

Description: Description\

In Migrations.sol, restricted() modifier is used to restrict some functions from being called by anyone. The functions which are restricted includes,

1) setCompleted() 2) upgrade()

restricted modifier is shown as,

    modifier restricted() {
        if (msg.sender == owner) _;
    }

The issue is, the modifer does not use strict checks and it does not revert if other than the owner calls the restricted functions.

I have tried the restricted modifier behaviour in Remix IDE and its surprise that any account calling the restricted functions will be successfull executed without any revert message, however the catch here is that the state variable value wont be changed. Its just a fake transaction getting successful without change in state variables and revert message if called other than contract owner.

Recommendation to fix\

Throw revert message of restricted functions are called other than owner. This will also save the gas for other accounts calling such functions and function will revert at start itself.

+  error InvalidCaller();

    modifier restricted() {
-        if (msg.sender == owner) _;
+        if (msg.sender != owner) 
              revert InvalidCaller();
+_;
    }

[0xRizwan]

I explained the issue in description but still refer below POC steps,

1) Jack calls Migrations.setCompleted() or Migrations.upgrade(). 2) Since both Migrations.setCompleted() or Migrations.upgrade() functions are protected by restricted modifier, the function calls from Jack should fail but it will be successfully executed. 3) This is due to non-strict check in restricted modifier. The transaction would be real but there wont be any state change in contract. This is the weird behaviour of restricted modifier. 4) This would be like a prank with Jack who called both functions with intention to modify the state variables but he couldn't.

This issue is actually negative impacting the user experience as the function calls to restricted functions should be reverted at function start itself but it does not happen in current implementation. Therefore, as a part of rules, the issue is submitted as minor severity.

Now, if we see the ink smart contract, such issue is not present in ink based contracts. This can be checked here, ensure_owner is supposedly acting as modifier which reverts in case owner does not call it.

        pub fn ensure_owner(&self) -> Result<(), MigrationsError> {
            let caller = self.env().caller();
@>          if caller != self.owner {
@>              Err(MigrationsError::CallerNotOwner(caller))      @audit // reverts which is missing in solidity contract
            } else {
                Ok(())
            }
        }

[fbielejec]

no-op modifier that should revert, valid submission, minor issue