hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

Lack of Events for Critical Actions #22

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x82d86181721269509590a7235751901f4d9acbce5cbeb41d8a80b39c97e1afb7 Severity: low

Description:

Description

The ForkModule smart contract lacks event emissions for critical actions such as removing a submission from the registry. Specifically, the remove and tryRemove functions modify the state by marking submissions as removed, but these state changes are not logged through events. Event logs are crucial for tracking state changes, debugging, and maintaining transparency. Without these logs, it becomes challenging for external observers, such as off-chain applications to monitor the contract's behavior.

Attack Scenario

In a situation where an external system or dApp relies on event logs to track state changes (e.g., a UI displaying a list of registered submissions), the lack of event emissions could cause these systems to display outdated or incorrect information. Users may not realize that their submission has been removed until much later, leading to potential loss of service or participation in the protocol.

Revised Code

pragma solidity 0.8.20;

contract ForkModule {
. . .
    // Event declaration for removal actions
+   event SubmissionRemoved(address indexed submissionID, bool successful);

    function tryRemove(address _submissionID) external onlyV2 returns (uint40 expirationTime) {
        require(!removed[_submissionID], "removed!");

        (, uint64 submissionTime, , bool registered, , ) = proofOfHumanityV1.getSubmissionInfo(_submissionID);

        expirationTime = uint40(submissionTime).addCap40(submissionDuration);

        require(registered && block.timestamp < expirationTime && submissionTime < forkTime, "Not registered, expired or submitted after the fork!");

        removed[_submissionID] = true;
+       emit SubmissionRemoved(_submissionID, true); // Event emitted here
    }
. . .
}

Note: This includes the following functions in CrossChainProofOfHumanity contract too:

clesaege commented 2 months ago

Similar to https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/issues/12