Tricko - CouncilMember._retrieve() will revert due to improper use of the _target variable.

[sherlock-admin2]

Tricko

high

CouncilMember._retrieve() will revert due to improper use of the _target variable.

Summary

During the execution of CouncilMember._retrieve(), the _target address variable is employed twice. Initially, it is used as an argument for PRBProxy.execute(), and subsequently, it is utilized in abi.encodeWithSelector to generate the necessary calldata for invoking SablierV2ProxyTarget.withdrawMax(). However, these two usages require distinct _target addresses since they correspond to calls targeting different contracts. Due to this mismatch, all calls to CouncilMember._retrieve() will revert, affecting most of the CouncilMember functionality.

Vulnerability Detail

When CouncilMember._retrieve() is being executed, it first calls PRBProxy.execute(address target, bytes calldata data). The first argument is the address of the contract to which PRBProxy will delegatecall. For CouncilMember._retrieve(), this target address should be the SablierV2ProxyTarget contract address.

Subsequently, it uses abi.encodeWithSelector to generate the calldata for calling SablierV2ProxyTarget.withdrawMax(). However, from the SablierV2ProxyTarget implementation (shown below), it is evident that the target argument should be the address of the SablierV2Lockup contract.

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

As described earlier, CouncilMember._retrieve() employs the same _target address for both PRBProxy.execute() and SablierV2ProxyTarget.withdrawMax(). However, this is not correct, as the target argument for PRBProxy.execute() must be the address for the SablierV2ProxyTarget, whereas the target argument for SablierV2ProxyTarget must be the address for the SablierV2Lockup (the actual stream). Consequently, execute() will always revert, either because it attempts to call the non-existent withdrawMax(ISablierV2Lockup lockup, uint256 streamId, address to) selector in SablierV2Lockup or the non-existent withdrawMax(uint256 streamId, address to) selector in the SablierV2ProxyTarget contract.

The test suite does not encounter this issue because it employs a mock contract instead of actual implementations of PRBProxy and SablierV2ProxyTarget.

It's important to note that fixing this issue on the current version of CouncilMember is not possible by simply changing the _target variable, as one variable cannot suffice for the two different addresses needed.

Impact

All calls to CouncilMember._retrive() will revert. As a consequence it will be impossible to call CouncilMember.claim(), CouncilMember.mint(), CouncilMember.burn() and CouncilMember.removeFromOffice().

Code Snippet

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

Tool used

Manual Review

Recommendation

Add a new target variable to be set as the address for the SablierV2Lockup contract.

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 a dupp of 086; making it medium due to the same impact mentioned in there}