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

Lack of Check for Identical NFTDescriptor Setting, Resulting in Redundant Execution, Additional GAS Costs, and Potential Confusion in Communication with Off-Chain Tools (e.g., APIs, websites, databases).

theirrationalone

Summary

In the SablierFlowBase.sol file, there is an external function, SablierFlowBase::setNFTDescriptor, which allows an
admin to set or update the NFTDescriptor. While the function works as expected, it lacks optimization. Specifically, it
does not check if the new NFT Descriptor matches the current NFT Descriptor, which can lead to redundant function
executions and unnecessary GAS costs.

See the issue here:
SablierFlowBase::setNFTDescriptor issue:

Vulnerability Details

/// @inheritdoc ISablierFlowBase
function setNFTDescriptor(IFlowNFTDescriptor newNFTDescriptor) external override onlyAdmin {
// Effect: set the NFT descriptor.
IFlowNFTDescriptor oldNftDescriptor = nftDescriptor;
​
@> // missing check for identical NFTDescriptor
nftDescriptor = newNFTDescriptor;
​
// Log the change of the NFT descriptor.
emit ISablierFlowBase.SetNFTDescriptor({
admin: msg.sender,
oldNFTDescriptor: oldNftDescriptor,
newNFTDescriptor: newNFTDescriptor
});
​
// Refresh the NFT metadata for all streams.
emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}

Currently, the function lacks a check to prevent setting the NFTDescriptor to the existing value, leading to redundant
operations. Optimizing this with a simple check would ensure efficiency, prevent unnecessary gas expenditure, and
streamline contract execution.

Impact

Currently,

  • there's an unnecessary gas expenditure, and streamline contract execution.

  • Emitting two events unnecessarily:

    • Event: BatchMetadataUpdate

    • Event: SetNFTDescriptor

This redundancy can lead to off-chain tools—such as websites, APIs, and databases—receiving duplicate data, potentially
increasing costs associated with bandwidth usage and creating less efficient communication with the protocol.

Tools Used

Manual Review

Recommendations

Add a condition to check if the new NFTDescriptor is the same as the existing one, preventing redundant updates. Below
is the suggested modification to the setNFTDescriptor function:

  • Updated Code

In SablierFlowBase.sol:

...
​
/// @inheritdoc ISablierFlowBase
function setNFTDescriptor(IFlowNFTDescriptor newNFTDescriptor) external override onlyAdmin {
// Effect: set the NFT descriptor.
IFlowNFTDescriptor oldNftDescriptor = nftDescriptor;
​
+ if (newNFTDescriptor == oldNftDescriptor) {
+ revert Errors.SablierFlowBase_SameNftDescriptor(newNFTDescriptor, oldNftDescriptor);
+ }
​
nftDescriptor = newNFTDescriptor;
​
// Log the change of the NFT descriptor.
emit ISablierFlowBase.SetNFTDescriptor({
admin: msg.sender,
oldNFTDescriptor: oldNftDescriptor,
newNFTDescriptor: newNFTDescriptor
});
​
// Refresh the NFT metadata for all streams.
emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}
​
...

In Errors.sol:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.22;
​
import { UD21x18 } from "@prb/math/src/UD21x18.sol";
import { UD60x18 } from "@prb/math/src/UD60x18.sol";
​
+ import { IFlowNFTDescriptor } from "./../interfaces/IFlowNFTDescriptor.sol";
​
/// @title Errors
/// @notice Library with custom errors used across the Flow contract.
library Errors {
/*//////////////////////////////////////////////////////////////////////////
GENERICS
//////////////////////////////////////////////////////////////////////////*/
​
/// @notice Thrown when an unexpected error occurs during a batch call.
error BatchError(bytes errorData);
​
...
...
​
+ /// @notice Thrown when trying to set new NFT Descriptor equals to old NFT Descriptor.
+ error SablierFlowBase_SameNftDescriptor(IFlowNFTDescriptor newNFTDescriptor, IFlowNFTDescriptor oldNftDescriptor);
​
/// @notice Thrown when trying to set protocol fee more than the allowed.
error SablierFlowBase_ProtocolFeeTooHigh(UD60x18 newProtocolFee, UD60x18 maxFee);
​
/// @notice Thrown when trying to recover for a token with zero surplus.
error SablierFlowBase_SurplusZero(address token);
}
​
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 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.