A user can register an unlimited number of subscriptions
registerSubscription has no per-user cap, allowing a single subscriber to register unlimited entries and grow the subscribers array until getSubscribers exhausts gas (denial of service).
Description
A user can register an infinite number of times since there are no restrictions in place to prevent it. It is therefore possible to register a large number of subscriptions such that registerSubscription() and getSubscribers() functions will run out of gas causing a Denial of Service.
function registerSubscription(address _subscriber, uint256 _amount,uint256 _validUntil, uint256 _paymentInterval, address _erc20Token)public {require(_amount > 0, "Subscription amount is 0");require(_paymentInterval > 0, "Payment interval is 0");require(msg.sender == _subscriber, "Only the subscriber can register a subscription");ISubExecutor.SubStorage memory sub = ISubExecutor.SubStorage({amount: _amount,validUntil: _validUntil,validAfter: block.timestamp,paymentInterval: _paymentInterval,subscriber: _subscriber,initiator: address(this),erc20TokensValid: _erc20Token == address(0) ? false : true,erc20Token: _erc20Token});subscriptionBySubscriber[_subscriber] = sub;subscribers.push(_subscriber);}
Impact
Unbounded growth of the subscribers array exhausts gas in iteration, causing a Denial of Service on getSubscribers() and slowing down subscription registration.
Recommendation
Implement a requirement to ensure that the same user has not registered other subscriptions previously or consider the possibility of adding an upper bound limit on the maximum number of subscriptions.
Sample implementation:
+ mapping(address => bool) private subscriberStatus;function registerSubscription(address _subscriber, uint256 _amount,uint256 _validUntil, uint256 _paymentInterval, address _erc20Token)public {require(_amount > 0, "Subscription amount is 0");require(_paymentInterval > 0, "Payment interval is 0");require(msg.sender == _subscriber, "Only the subscriber can register a subscription");+ require(!subscriberStatus[_subscriber], "Subscriber already registered");subscriptionBySubscriber[_subscriber] = sub;+ subscriberStatus[_subscriber] = true;}
Resolution
Team Response: Acknowledged and fixed as suggested.
