Flow

Sablier
FoundryDeFi
20,000 USDC
View results
Submission Details
Severity: medium
Invalid

Event Emission Timing

Summary

The TransferAdmin event is emitted after updating the admin address in the transferAdmin function, which may lead to inconsistencies in event logs.

Finding Description

In the transferAdmin function of the Adminable contract, the event TransferAdmin is emitted after the state variable admin is updated. This can lead to situations where event listeners receive an event that reflects the state of the contract before the change occurs. This can cause confusion and potentially allow external systems or users to react to outdated information regarding the admin address.

While this issue does not pose a direct security vulnerability, it undermines the reliability and predictability of contract behavior, which are critical components of blockchain applications. In scenarios where external services rely on event logs for state updates, this could lead to significant misinterpretations and faulty assumptions about the current admin status.

Vulnerability Details

  • Affected Function: transferAdmin

  • Code Snippet:

    function transferAdmin(address newAdmin) public virtual override onlyAdmin {
    // Effect: update the admin.
    admin = newAdmin;
    // Log the transfer of the admin.
    emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
    }
  • The event is emitted after the admin state variable is updated, which can lead to outdated event information being logged.

Impact

The impact of this issue is classified as Medium because:

  • Although it does not pose an immediate security risk, it affects the integrity of event emissions.

  • External systems relying on these events may act on stale data, leading to erroneous behavior.

  • Users and developers expect event logs to accurately reflect state changes at the time of emission, and failing to meet this expectation can erode trust in the contract's behavior.

Proof of Concept

To illustrate the issue, consider a scenario where an external service listens for the TransferAdmin event to update its records. If the event is logged after the state change, this service would see the old admin address in the event, leading to a mismatch with the actual contract state:

Current Admin: 0x123...abc
Event Emitted: TransferAdmin(0x123...abc, 0x456...def) // Incorrect oldAdmin

Recommendations

To fix the event emission timing issue, the event should be emitted before the state variable is updated. This ensures that the event log reflects the correct state of the contract at the time the action is taken.

Suggested Code Fix:

function transferAdmin(address newAdmin) public virtual override onlyAdmin {
require(newAdmin != address(0), "New admin cannot be the zero address");
// Log the transfer of the admin before updating the state.
emit IAdminable.TransferAdmin({ oldAdmin: admin, newAdmin: newAdmin });
// Effect: update the admin.
admin = newAdmin;
}

File Location

Adminable.sol

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.