sherlock-audit / 2024-05-sophon-judging

1 stars 1 forks source link

Early_Emission #235

Closed sherlock-admin2 closed 1 month ago

sherlock-admin2 commented 1 month ago

Early_Emission

Low/Info issue submitted by petarP1998

Summary

In the Upgradeable2Step::acceptImplementation function, the event ReplaceImplementation is emitted before the state is updated. This order of operations can potentially cause issues with event listeners and logging, as the event does not accurately reflect the state at the time of its emission.

Vulnerability Detail

The acceptImplementation function in the Upgradeable2Step contract is responsible for updating the implementation address to the msg.sender if it matches the pendingImplementation. However, the event ReplaceImplementation is emitted before the implementation address is actually updated. This means that the event log will show the old state rather than the new state immediately, which can lead to inconsistencies in the system's audit trail and make debugging more difficult.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/proxies/Upgradeable2Step.sol#L22-L30

Impact

Emitting the event before the state update can lead to a mismatch between the logged events and the actual contract state. This can cause issues for off-chain services and monitoring tools that rely on these events to accurately reflect the contract's state changes. It can also complicate the debugging and auditing processes, as the event logs do not represent the true state transitions of the contract.

Code Snippet

Original code:

function acceptImplementation() public {
    if (msg.sender != pendingImplementation) {
        revert OwnableUnauthorizedAccount(msg.sender);
    }
    emit ReplaceImplementation(implementation, msg.sender);
    delete pendingImplementation;
    implementation = msg.sender;
}

Tool used

Manual Review

Recommendation

To ensure that the event accurately reflects the state of the contract at the time of emission, move the emit ReplaceImplementation statement after the state has been updated. This ensures that the logged event matches the actual state transition.

Revised acceptImplementation function:

function acceptImplementation() public {
    if (msg.sender != pendingImplementation) {
        revert OwnableUnauthorizedAccount(msg.sender);
    }
    address oldImplementation = implementation;

    delete pendingImplementation;
    implementation = msg.sender;

    emit ReplaceImplementation(oldImplementation, msg.sender);
}