Missing events for functions that change critical parameters

[hats-bug-reporter[bot]]

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

Description: The restricted functions that change critical parameters should emit events.

1. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.
2. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
3. Missing events do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness.

In Migrations.sol, below are restricted functions that do not emit any events in the contracts.

    function setCompleted(uint256 completed) public restricted {
        last_completed_migration = completed;
    }

    function upgrade(address new_address) public restricted {
        Migrations upgraded = Migrations(new_address);
        upgraded.setCompleted(last_completed_migration);
    }

Recommendation\ Add events to all restricted functions that change critical parameters.

[0xRizwan]

This issue is also found in ink! Migration contract which can be checked here.

[fbielejec]

invalid submission - missing POC of an attack, submitted issue is a subjective opinion rather than a real vulnerability

[0xRizwan]

@fbielejec

Missing events issue does not need an attack POC. I would request to check the 3 points mentioned in description as it justifies the need of events.

The issue is in scope as it falls in contest rules and its submitted as Minor severity.

Attacks not mentioned in higher categories, but having a negative impact on user experience, or putting one of the contracts in an unexpected state.

[fbielejec]

Correct - and users do not process events, only processes/machines do. As far as events are concerned emitting them costs gas (money), therefore unless the architecture calls for it emitting an event is just an additional cost that burdens the actual users. I would turn your argument around and say that emitting events that are not strictly neccessary for the protocol to operate, or to satisfy an erc/psp are a type of an issue/vulnerability

[0xRizwan]

@fbielejec Agreed on your cost point. In this case, the function is only called by contract owner so event emission is one time cost depends on calling of setCompleted(). The event will give information that particular migration is completed. This information should be emitted on blockchain for users as well as owners information.

Historically, events are a low severity issues but as a part of rules, the issue still deserves minor severity. Hope you reconsider the decision on validation of this issue. I would respect your final decision without further arguments. Thanks.