Change sequence of extendPromotion operations

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The extendPromotion() function in TwabRewards.sol extends the promotion before the safeTransferFrom() call. It would be better if the safeTransferFrom() call occurs before the promotion epochs is extended to revert the safeTransferFrom() function earlier if the msg.sender has insufficient funds. This change would only save gas when a revert condition occurs. As a matter of pratice, it would be better to make sure that the msg.sender pays for the promotion extension BEFORE the promotion is extended, because the current code sequence has a user pay AFTER the promotion is extended.

Proof of Concept

The extendPromotion function of TwabRewards.sol: https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L141-L159

Tools Used

Manual analysis

Recommended Mitigation Steps

Move lines 153-154 of TwabRewards.sol up before the 2 lines of code in the same function that extend the promotion duration to reduce the operations when a revert condition is encountered: https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L153-L154

[PierrickGT]

As a best practice, it is actually better to call transfer functions after most logic code, here the extension of the promotion, to avoid any reentrancy attack that could be performed by using a malicious token. For this reason, I have disputed the issue.

[dmvt]

Reasonable dispute