sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

0xVinylDavyl - The absence of event emission in error cases can significantly impede the maintenance, debugging and monitoring process. #15

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

0xVinylDavyl

medium

The absence of event emission in error cases can significantly impede the maintenance, debugging and monitoring process.

Summary

The code lacks event emission in error cases, which hinders the ability to track and diagnose contract issues effectively.

Vulnerability Detail

error cases within functions like _commitPrice do not emit events. This omission makes it challenging to identify when and why errors occur during contract execution. Emitting events in error cases is a fundamental best practice in smart contract development for debugging and monitoring.

Impact

The absence of event emission in error cases can significantly impede the debugging and monitoring process. Future Developers and users will have difficulty pinpointing the cause of errors or unexpected behavior, which can lead to delays in diagnosing and addressing issues.

Code Snippet

function _commitPrice(
    address oracleProvider,
    uint256 value,
    uint256 index,
    uint256 version,
    bytes memory data,
    bool revertOnFailure
) internal {
    UFixed18 balanceBefore = DSU.balanceOf();

    if (revertOnFailure) {
        IPythOracle(oracleProvider).commit{value: value}(index, version, data);
    } else {
        try IPythOracle(oracleProvider).commit{value: value}(index, version, data) { }
        catch { }
    }

    // Return through keeper reward if any
    DSU.push(msg.sender, DSU.balanceOf().sub(balanceBefore));
}

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L320

Tool used

Manual Review

Recommendation

To enhance contract visibility and facilitate debugging, it's recommended to emit events in error cases. Here's an example of how to modify the _commitPrice function to emit an event in case of an error:

event CommitError(address indexed oracleProvider, uint256 index, uint256 version, bytes data);

function _commitPrice(
    address oracleProvider,
    uint256 value,
    uint256 index,
    uint256 version,
    bytes memory data,
    bool revertOnFailure
) internal {
    UFixed18 balanceBefore = DSU.balanceOf();

    if (revertOnFailure) {
        try IPythOracle(oracleProvider).commit{value: value}(index, version, data) { }
        catch {
            // Emit an error event
            emit CommitError(oracleProvider, index, version, data);
            revert("Commit failed");
        }
    } else {
        try IPythOracle(oracleProvider).commit{value: value}(index, version, data) { }
        catch {
            // Emit an error event
            emit CommitError(oracleProvider, index, version, data);
        }
    }

    // Return through keeper reward if any
    DSU.push(msg.sender, DSU.balanceOf().sub(balanceBefore));
}

By emitting an event (in this case, CommitError) in error cases, you provide valuable information for tracking and diagnosing issues within the contract. This practice is crucial for maintaining contract integrity and security.

sherlock-admin commented 1 year ago

5 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid because event emits are not valid issues by sherlock rules

n33k commented:

invalid, ai generated groundless report

0xyPhilic commented:

invalid because it can be considered informational

darkart commented:

The same as 14

polarzero commented:

Invalid. The lack of event emission, as in making it more challenging to monitor the contract, is not severe enough to be considered here.