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.
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().
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 forPRBProxy.execute()
, and subsequently, it is utilized inabi.encodeWithSelector
to generate the necessary calldata for invokingSablierV2ProxyTarget.withdrawMax()
. However, these two usages require distinct_target
addresses since they correspond to calls targeting different contracts. Due to this mismatch, all calls toCouncilMember._retrieve()
will revert, affecting most of theCouncilMember
functionality.Vulnerability Detail
When
CouncilMember._retrieve()
is being executed, it first callsPRBProxy.execute(address target, bytes calldata data)
. The first argument is the address of the contract to whichPRBProxy
willdelegatecall
. ForCouncilMember._retrieve()
, this target address should be theSablierV2ProxyTarget
contract address.Subsequently, it uses
abi.encodeWithSelector
to generate the calldata for callingSablierV2ProxyTarget.withdrawMax()
. However, from theSablierV2ProxyTarget
implementation (shown below), it is evident that the target argument should be the address of theSablierV2Lockup
contract.As described earlier,
CouncilMember._retrieve()
employs the same_target
address for bothPRBProxy.execute()
andSablierV2ProxyTarget.withdrawMax()
. However, this is not correct, as the target argument forPRBProxy.execute()
must be the address for theSablierV2ProxyTarget
, whereas the target argument forSablierV2ProxyTarget
must be the address for theSablierV2Lockup
(the actual stream). Consequently,execute()
will always revert, either because it attempts to call the non-existentwithdrawMax(ISablierV2Lockup lockup, uint256 streamId, address to)
selector inSablierV2Lockup
or the non-existentwithdrawMax(uint256 streamId, address to)
selector in theSablierV2ProxyTarget
contract.The test suite does not encounter this issue because it employs a mock contract instead of actual implementations of
PRBProxy
andSablierV2ProxyTarget
.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 callCouncilMember.claim()
,CouncilMember.mint()
,CouncilMember.burn()
andCouncilMember.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