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

Inefficient Event Emissions Leading to Increased Gas Costs in SablierFlowBase

hackz

Summary

The contract emits events unconditionally in certain functions, which can lead to unnecessary gas costs and increased transaction fees for users.

Finding Description

In the collectProtocolRevenue, setNFTDescriptor, and setProtocolFee functions, events are emitted without verifying if there are significant state changes that warrant an event emission. This can lead to unnecessary on-chain activity and higher costs for users, especially if these functions are called frequently without substantial changes in state.

This issue breaks the security guarantees regarding gas efficiency and usability by creating a situation where users may incur higher transaction costs without receiving additional value. For example, if a user collects protocol revenue but no revenue is available, the event should not be emitted, as it doesn't reflect a meaningful change.

Vulnerability Details

  • The collectProtocolRevenue function emits the CollectProtocolRevenue event regardless of whether the protocol revenue was non-zero before the transfer, potentially leading to multiple events with the same data when no actual revenue was collected.

  • The setNFTDescriptor and setProtocolFee functions emit metadata updates even if the NFT descriptor or protocol fee does not change, leading to unnecessary emissions.

Impact

I’ve assessed the impact as medium because while this issue does not lead to security vulnerabilities like fund loss or reentrancy, it does negatively affect the cost-efficiency of the contract and could deter users from engaging with the contract due to higher transaction fees. High gas fees can significantly reduce the contract’s usability, especially in scenarios with frequent function calls.

Proof of Concept

Here is an example of the current implementation that showcases the unnecessary emissions:

function collectProtocolRevenue(IERC20 token, address to) external override onlyAdmin {
uint128 revenue = protocolRevenue[token];
​
// Check: there is protocol revenue to collect.
if (revenue == 0) {
revert Errors.SablierFlowBase_NoProtocolRevenue(address(token));
}
​
// Effect: reset the protocol revenue.
protocolRevenue[token] = 0;
​
unchecked {
// Effect: update the aggregate balance.
aggregateBalance[token] -= revenue;
}
​
// Interaction: transfer the protocol revenue to the provided address.
token.safeTransfer({ to: to, value: revenue });
​
// Emitting event even if revenue was initially zero.
emit ISablierFlowBase.CollectProtocolRevenue({ admin: msg.sender, token: token, to: to, revenue: revenue });
}

Recommendations

To address this issue, I recommend adding checks to ensure that events are emitted only when there is a meaningful change in state. For example:

function collectProtocolRevenue(IERC20 token, address to) external override onlyAdmin {
uint128 revenue = protocolRevenue[token];
​
// Check: there is protocol revenue to collect.
if (revenue == 0) {
revert Errors.SablierFlowBase_NoProtocolRevenue(address(token));
}
​
// Effect: reset the protocol revenue.
protocolRevenue[token] = 0;
​
unchecked {
// Effect: update the aggregate balance.
aggregateBalance[token] -= revenue;
}
​
// Interaction: transfer the protocol revenue to the provided address.
token.safeTransfer({ to: to, value: revenue });
​
// Emit the event only if there was actual revenue collected.
if (revenue > 0) {
emit ISablierFlowBase.CollectProtocolRevenue({ admin: msg.sender, token: token, to: to, revenue: revenue });
}
}

Additionally, similar checks should be added to the setNFTDescriptor and setProtocolFee functions to ensure events are only emitted when actual changes occur.

File Location

SablierFlowBase.sol

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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