Arz - Wrong Sablier contract is being called in _retrieve()

[sherlock-admin2]

Arz

medium

Wrong Sablier contract is being called in _retrieve()

Summary

When _retrieve() is called we first call the PRBProxy which delegate calls a Sablier ProxyTarget which then calls the Sablier lockup. the problem here is that the wrong _target address is being used to call withdrawMax()

Vulnerability Detail

To withdraw from the Sablier stream we first need to call the PRBProxy which is the recipient of the stream, this proxy will then delegate call SablierV2ProxyTarget which will then call withdrawMax() on the lockup contract.

Lets take a look at how this gets called in the _retrieve() function:

_stream.execute(
            _target,
            abi.encodeWithSelector(
                ISablierV2ProxyTarget.withdrawMax.selector,
                _target,
                _id,
                address(this)
            )
        );

_stream is the proxy contract which will then delegate calls the _target which is the ProxyTarget, lets take a look at withdrawMax() in the SablierV2ProxyTarget :

function withdrawMax(ISablierV2Lockup lockup, uint256 streamId, address to) external onlyDelegateCall {
       lockup.withdrawMax(streamId, to);
}

As you can see this will call the lockup address that was passed in but the problem is that in _retrieve() we have passed in _target as the argument encoded with the selector which is wrong because we need the address of the lockup. This will then call withdrawMax(streamId, to) on the ProxyTarget and because this function does not exist the call will fail.

Impact

Withdrawing from the Sablier stream will always fail and _retrieve() will always fail so the CouncilMember contract will be unusable because _retrieve() is called in almost every function.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L275C25-L275C25

Tool used

Manual Review

Recommendation

Pass in the address of the Sablier lockup not the TargetProxy.

_stream.execute(
            _target,
            abi.encodeWithSelector(
                ISablierV2ProxyTarget.withdrawMax.selector,
                _lockup,                                                              <= HERE
                _id,
                address(this)
            )
        );

Duplicate of #139

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { This is valid and i cosndier it a high due to the fact that main funcitonality will fail; the watson explain how using _target instead of lookUp address causes a problem due to non existent of the called function in the used _target address}