The CouncilMember contract will be completely broken because the Sablier stream reverts when withdrawing 0 amounts

Summary

If withdrawMax() is called and the withdrawable amount is 0 the Sablier contract will revert. This will DOS most functions in CouncilMember.sol because _retrieve() is sometimes called 2 times(for example in burn(), it gets called in update() the second time) and the withdrawable amount will be 0 once _retrieve() is called the second time.

Vulnerability Detail

If withdrawMax() in the Sablier contract is called and the withdrawable amount is 0 the call will revert because there is a 0 amount check. See here.

This will be a big problem for:

Functions where _retrieve() is called 2 times - mint(),burn() and removeFromOffice(). _retrieve() is first called in the function and then its called the second time in ERC721.update() which will fail because we already withdrew the max . This applies to any stream types
Dynamic stream types for example vesting with periodic unlocks where the withdrawable amount will be 0 for some time.

This will completely break the contract and mint(),burn() and removeFromOffice() will be DoSed right after the first mint. If a stream is used where the withdrawable amount is 0 for some time, council members will be unable to claim their allocated amounts because withdrawing 0 will revert so calling claim() will fail.

Impact

The contract will be completely unusable and council member can fail to receive their allocated amounts of TEL.

Code Snippet

https://github.com/sablier-labs/v2-core/blob/b0016437ef3cc8606e1100965dd911d7e658b40b/src/abstracts/SablierV2Lockup.sol#L270

270:  if (amount == 0) {
271:        revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
272:  }

As you can see in the Sablier contracts when withdrawMax() is called and the amount is 0, the tx reverts.

When we call mint(), _retrieve() is first called in the function and then _mint() calls _update()

function mint(
        address newMember
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
        if (totalSupply() != 0) {
            _retrieve();
        }

        balances.push(0);
        _mint(newMember, totalSupply());
    }

_update() will then call _retrieve() again but this time the withdrawable amount will be 0 and the tx will revert.

function _update(
        address to,
        uint256 tokenId,
        address auth
    ) internal override returns (address) {
        if (totalSupply() != 0) {
            _retrieve();
        }

        return super._update(to, tokenId, auth);
    }

Tool used

Manual Review

Recommendation

Before calling withdraw check if the withdrawable amount is 0.

if(sablier.withdrawableAmountOf(_id) > 0) {
    // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol
    _stream.execute(
    ...
}

Duplicate of #47

sherlock-audit / 2024-01-telcoin-judging

Arz - The CouncilMember contract will be completely broken because the Sablier stream reverts when withdrawing 0 amounts #98