Wrong `modifier()` implemented in `Migrations.sol`

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

Description: Description\ In the Migrations.sol file, there's a modifier named restricted() that's supposed to prevent certain functions from being called by anyone except the contract owner. However, there's an issue with this modifier. Currently, it doesn't perform strict checks, meaning it allows any account to call the restricted functions without causing a revert. Although the transaction appears successful, it doesn't actually change the state variables.

To rectify this issue, it's recommended to modify the restricted() modifier to include strict checks and revert the transaction.

PoC

here is step by step process:

Bob is normal user (not owner)
He calls setCompleted()
It should revert because this function have a restricted modifier that checks caller should be owner
but transaction will execute because restricted modifier does not work

if we check restricted modifier code:

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

we see that modifier checks msg.sender==owner, but the issue is it doesn't do anything with it so it means whether msg.sender is owner or not the transaction will execute successfully. it should revert in the case that caller is not owner.

same scenario for upgrade() function.

Reccomendation

Here's a suggested fix:

// Define a custom error message for unauthorized callers
error UnauthorizedAccess();

// Modify the restricted modifier to perform strict checks
modifier restricted() {
    // Only allow the owner to proceed, otherwise revert with an error message
    if (msg.sender != owner) 
        revert UnauthorizedAccess();
    // If the caller is the owner, continue with the function execution
    _;
}

hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Wrong `modifier()` implemented in `Migrations.sol` #24

PoC

Reccomendation