Turtle Club Protocol

Description

In rescueTokensFromStream() function in StreamFactory.sol, the following event is emitted :

function rescueTokensFromStream(        IStream stream,        IERC20 token,        address payable to,        uint amount    ) external restricted validStream(stream) {        stream.rescueTokens(token, to, amount);        emit TokensRescuedFromStream(stream, token, to, amount);

Here, the amount is passed on as function input and emitted as it is. But we can see in Stream.rescueTokens() logic that actual amount that is rescued can be different from the input amount.

if (balance < amount) amount = balance;

In this edge case, the amount emitted in TokensRescuedFromStream() is incorrect.

Recommendation

Consider modifying rescueTokensFromStream() function such that it emits the actual amount of the token rescued.

Description

In Stream.sol, the stream owner is allowed to withdraw the rewardToken balance, but it is not checked anywhere if the to address provided is a valid address.

If the owner uses address(0) as the to address by mistake, the full rewardToken balance will be lost.

The withdrawal flow from StreamFactory => Stream.withdrawFunds() also lacks this check.

Note : Not all tokens revert on transferring to address(0), some tokens allow this.

Recommendation

Consider adding this check to Stream.sol::withdrawFunds(), just like how its done in Stream.sol::rescueTokens()

if (to == address(0)) revert InvalidToAddress();

Description

There are several instances of misleading/ incorrect comments across the codebase :

• StreamFactory.sol #L66 : Mapping to record stream info by stream ID should be changed to => Mapping to record stream info by stream address
• StreamFactory.sol #L68 : Array of all created stream IDs => Array of all created stream addresses
• StreamFactory.sol #L70 : Mapping to map stream contract address to stream ID => Mapping to map stream ID to stream contract address
• StreamFactory.sol #L98 : only the admin or authorized roles can unpause => only the admin or authorized roles can pause, unpause and changeStreamAdmin

Recommendation

Consider modifying the comments as noted.

Description

The Stream.sol contract inherits from OpenZeppelin's Ownable2StepUpgradeable library, and has an owner address responsible for important admin operations.

For standalone stream instances deployed independently of the factory, the Stream owner will be controlled by the stream admin. If renounceOwnership() is called by current owner by mistake, this can lead to permanent loss of ownership, which can create issues with the stream's operations.

Recommendation

Consider overriding renounceOwnership() in Stream.sol to always revert.

Description

In StreamFactory.sol, the protocol admin can set a new stream implementation address, which will be used in future deployments of new stream clones. This can be done using setStreamImplementation() function.

But the new implementation might have different initialization logic, preventing compatibility with the current StreamFactory. This is because when initializing the Stream clone in createStream(), the initData passed to the new stream clone is hardcoded in current factory.

function _createStream(        CreateStreamParams calldata params,        uint40 deadline,        bytes calldata signature    ) private returns (IStream stream) {        StreamFactoryStorage storage $ = _getStreamFactoryStorage();
        /*** Deploy stream ***/        // Encode initialization data        bytes memory initData = abi.encodeWithSelector(            IStream.initialize.selector,            params.streamId,            params.rewardToken        );                // Deploy proxy        stream = IStream(address(new ERC1967Proxy(address($.streamImplementation), initData)));

If protocol were to create a new stream implementation which has a different selector for its initialize() function, all new stream clone deployments would fail due to selector mismatch.

Recommendation

Consider documenting the limitations of the approach used for upgrading stream implementation in the current factory, as some types of modifications in the stream logic will not work with the current factory version.

Description

The interface documentation for batchClaim and batchClaimFor states @dev Modifiers: nonReentrant. However, the corresponding implementations in StreamFactory do not apply any nonReentrant guard (nor an equivalent custom reentrancy protection).

This creates an inconsistency between the interface’s NatSpec and the actual runtime behavior.

Recommendation

Consider to either:

• remove/update the outdated NatSpec comment in IStreamFactory to match the implementation, or
• add an explicit reentrancy guard in StreamFactory if the original design intent was to enforce nonReentrant semantics for these entrypoints.

Description

The contract Stream imports ReentrancyGuardUpgradeable (src/Stream.sol:7), but the contract does not inherit from it nor use any of its modifiers.

Based on the client’s clarification, this was an intentional design decision after determining that no reentrancy risks are present, and the guard is therefore unnecessary.

Recommendation

Consider to remove the unused ReentrancyGuardUpgradeable import to keep the codebase minimal and aligned with the intended design.

Description

In createStream() function, the signature is verified first and input validation is done later for the CreateStreamParams.

function createStream(        CreateStreamParams calldata params,        uint40 deadline,        bytes calldata signature    ) external returns (IStream stream) {        _validateCreateStreamSignature(params, deadline, signature);        _validateCreateStreamParams(params);

Checking malformed input params first will potentially save gas, so we can re-arrange the logic to first check _validateCreateStreamParams(), followed by _validateCreateStreamSignature().

Recommendation

Consider checking CreateStreamParams first, before trying to verify the signature.