_getPromotion() doesn't revert on invalid _promotionId

[code423n4]

Handle

johnnycash

Vulnerability details

Impact

_getPromotion() doesn't revert if the specified _promotionId doesn't exist. It can lead to unexpected behaviors in callers of this function.

For instance, claimRewards will continue its execution and call _calculateRewardAmount() and eventually _promotion.token.safeTransfer() (probably with _rewardsAmount equal to 0).

Analysis

In contrary to the following comment:

@dev Will revert if the promotion does not exist.

_getPromotion() doesn't revert if the specified _promotionId doesn't exist, but return a Promotion structure with all fields set to 0.

Tools Used

Text editor.

Recommended Mitigation Steps

Fix suggestion:

    function _getPromotion(uint256 _promotionId) internal view returns (Promotion memory _promotion) {
        _promotion = _promotions[_promotionId];
        require(_promotion.creator != address(0), "TwabRewards/invalid-promotion");
        return _promotion;
    }

[PierrickGT]

This is an issue we are going to fix but I think it should be of severity 1 (Low Risk) and not 2 (Med Risk).

[dmvt]

Agree with the sponsor regarding risk rating. This is a comment issue in an internal function.

1 — Low (L): vulns that have a risk of 1 are considered “Low” severity when assets are not at risk. Includes state handling, function incorrect as to spec, and issues with comments.