0xadrii - Functions calling _retrieve() more than once will always revert

[sherlock-admin]

0xadrii

high

Functions calling _retrieve() more than once will always revert

Summary

All the functions that execute the _retrieve() function more than once in their execution flow will fail because all calls to Sablier's withdrawMax() function will try to withdraw a 0 amount.

Vulnerability Detail

CouncilMember.sol uses the _retrieve() function in order to withdraw the current available amount in the Sablier stream.

In order to do so, the _retrieve() will call withdrawMax() in the proxy target, a contract which will internally trigger the stream’s withdrawMax() function:

// CouncilMember.sol

function _retrieve() internal {
        // Get the initial TELCOIN balance of the contract
        uint256 initialBalance = TELCOIN.balanceOf(address(this));
        // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol
        _stream.execute(
            _target,
            abi.encodeWithSelector(
                ISablierV2ProxyTarget.withdrawMax.selector, 
                _target, 
                _id,
                address(this)
            )
        );

        ...
    }

// https://github.com/sablier-labs/v2-periphery/blob/ba3926d2c3e059a230211077087b73afe46acf64/src/abstracts/SablierV2ProxyTarget.sol#L142
// SablierV2ProxyTarget.sol
function withdrawMax(ISablierV2Lockup lockup, uint256 streamId, address to) external onlyDelegateCall {
        lockup.withdrawMax(streamId, to);
    }

When lockup.withdrawMax(), the actual Sablier lockup linear stream’s withdrawMax() will be called. This function will withdraw the maximum withdrawable amount available in the stream, which is given by the internal _withdrawableAmountOf() function:

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

function withdrawMax(uint256 streamId, address to) external override {
        withdraw({ streamId: streamId, to: to, amount: _withdrawableAmountOf(streamId) });
    }

As we can see in the snippet, withdrawMax() will then execute withdraw(), the actual stream’s function that will carry out the logic to transfer the available amount in the stream. If we analyze the withdraw() function, we’ll notice that it will always revert if a 0 amount is passed as parameter.

The problem with the current Telcoin implementation: because some functions call the _retrieve() function twice, all of them will fail because the first call to _retrieve() will withdraw ALL the available funds. After performing the first retrieve, the second one will always try to withdraw a 0 amount because everything has been withdrawn in the first retrieval.

This affects the removeFromOffice(), mint()and burn() functions. All of them perform only one explicit call to _retrieve(), but are then also affected by the changes added by the Telcoin team in the ERC721’s _update() hook:

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

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

This hook forces all NFT minting, burning or transfers to trigger a _retrieve(), which are actions performed by the three mentioned functions.

Impact

High. Calling the removeFromOffice(), mint()and burn() functions after totalSupply() > 1 (i.e after the first NFT is minted) will always revert, effectively rendering the protocol unusable.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L129

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L177

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L215

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L327

Tool used

Manual Review

Recommendation

It is recommended to only execute the _retrieve() if the stream’s withdrawable amount is greater than 0 (i.e there’s something to withdraw). This can be easily queried in the stream's withdrawableAmountOf() function.

Duplicate of #47

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/37

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/37.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.