Summary

Admin would be Unable to withdraw unclaimed tokens from the campaigns that has duly ended.

Vulnerability Details

In the SablierV2MerkleLockup.sol contract, the function clawback(address to, uint128 amount) external, which according to the NatSpec is supposed to be called by the protocol Admin to withdraw unclaimed token from campaigns that has ended with grace period over and the campaign past its expired date, would not be fit for purpose as it would always revert when it should initiate claw backs. The problem is that whilst the function tries to ensure that the said campaign is well over before initiating a withdrawal, the if statement at the beginning of the function would always revert especially when the campaigns is actually over if (_hasGracePeriodPassed() && !hasExpired()). The issue is the funtion called to check if campaigns has expired (function hasExpired() public view override returns (bool)), returns a false if the EXPIRATION > 0 && EXPIRATION <= block.timestamp; signafying that the campaign is still ongoing. However once this conditions is true EXPIRATION > 0 && EXPIRATION <= block.timestamp meaning the campaign has ended, the logic in the 'clawback functionnegates the result of thehasExpired() function, ensuring it returns false when condition is true. This ensures that the if condition in the clawback() (if (_hasGracePeriodPassed() && !hasExpired())`) always reverts and Admin unable to make the withdrawals from the expired campaigns.

Impact

The protocol would be unable to recover unclaimed tokens from the expired campaigns as set out in the first place creating the clawback function. The impact would be the protocol losing money or money stuck in contract, which should have easily been claimed back.

Tools Used

Manual Review

Recommendations#

The if statement in the clawback function needs to be amended to take the unary negation on the return value hasExpired():

This ensures the function reverts even though campaign would have ended.