eeshenggoh - Lack of check if proxy executed was successful leading to inaccurate update of rewards

[sherlock-admin]

eeshenggoh

high

Lack of check if proxy executed was successful leading to inaccurate update of rewards

Summary

CouncilMember::_retrieve() is used to retrieve and distribute TELCOIN to council members based on the stream from _target. It uses a Sabiler PRBproxy to withdraw tokens to the CouncilMembers contract. The problem lies in not checking whether the transaction is successful.

Vulnerability Detail

The CouncileMember.sol::retrieve() calls the IPRBProxy.sol::execute() which performs a delegate call to withdraw tokens. In the IPRBProxy.sol from Etherscan execute function's Natspec states that It returns the data it gets back, and bubbles up any potential revert. This means that, if the target contract's function being called through execute encounters a revert, that revert is not caught or handled within the execute function itself. Instead, it is allowed to propagate back to the caller of the execute function, and the caller can decide how to handle or react to the revert.

Impact

The retrieval and distribution of TELCOIN will be inaccurate hence, causing users to lose their TELCOIN rewards.

Code Snippet

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

Tool used

Manual Review

Recommendation

1. Handle the (bytes memory response) received

bytes memory response = _stream.execute(
    _target,
    abi.encodeWithSelector(
        ISablierV2ProxyTarget.withdrawMax.selector,
        _target,
        _id,
        address(this)
    )
);
// Custom Error helps saves gas
error ExecuteFail();

// Check if the response indicates a revert
if (response.length > 0) {
    // Handle revert
    // You may parse the response to get the revert reason
    // Note: The exact method depends on the implementation of your contracts
    revert ExecuteFail();
}

[amshirif]

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

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid because the "buble up" here means to reshurfle and return a revert with the returned data; read}

[goheesheng]

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

takarez commented:

invalid because { This is invalid because the "buble up" here means to reshurfle and return a revert with the returned data; read}

The vulnerability here is that it isn't handled with the reverted data at all. This means that since it isn't handled the data price will always be staled. Since, it always gets reverted without exception, the protocol final balance will remain the same, causing each player to not get the deducted rewards distribution. Hence protocol loses lots of funds because it is permanently staled data and will never updated. Therefore it is clear it is a high vuln.

[nevillehuang]

@goheesheng @amshirif In what scenarios will a revert be triggered? I will have to look more into the out of scope proxy contract mentioned here to ensure this did not arise because of user input error.

[nevillehuang]

request poc

[sherlock-admin2]

PoC requested from @goheesheng

Requests remaining: 6

[goheesheng]

1. _retrieve internal functions is called.
2. Withdrawal not executed and protocol funds are not withdraws to the contract.
3. When user calls claim, the funds rewards are not even distributed from the protocol rewards earned assuming that the Sabliers has any rewards yielded.
4. Since final balance == running balance which means that is rewards that cannot be distributed.
5. Users doesn't get any rewards at all.

Have confirmed with developer from Sablier team Andreivladbrg.

[nevillehuang]

@goheesheng Can you share the discussion between you and sablier?

[amshirif]

@nevillehuang the above by @goheesheng is correct

[goheesheng]

@goheesheng Can you share the discussion between you and sablier?

"The transaction would fail"

[0xLogos]

@amshirif @goheesheng Assuming call can silently fail without revert. Exactly where and which funds are lost? How propesed solution helps?

[0xLogos]

Escalate
Invalid
Proposed solution (reverting) doesn't make any difference

[sherlock-admin]

Escalate
Invalid
Proposed solution (reverting) doesn't make any difference

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xf1b0]

I agree with the escalation. The proposed solution doesn't make any difference, as the call already reverts.
Furthermore, it's exactly what we don't want, as it results in a DoS, as described in #139 and #47.

[goheesheng]

Silent reverts can be variant of a DoS but it will return a value which can be checked via the if statement

[goheesheng]

I agree with the escalation. The proposed solution doesn't make any difference, as the call already reverts.
Furthermore, it's exactly what we don't want, as it results in a DoS, as described in #139 and #47.

Also using the #139 is using stream directly and hence changing the address. But there isn't a check for the returned value, hence it's clearly non duplicate of #139.

[0xArz]

This is clearly not a valid issue as the _execute() checks if the delegatecall was succesful or no, see here:

https://etherscan.io/address/0x584009E9eDe26e212182c9745F5c000191296a78#code#F5#L123
https://github.com/PaulRBerg/prb-proxy/blob/fa13cf09fbf544a2d575b45884b8e94a79a02c06/src/PRBProxy.sol#L69-L79

if (!success) {
            // If there is return data, the delegate call reverted with a reason or a custom error, which we bubble up.
            if (response.length > 0) {
                assembly {
                    // The length of the data is at `response`, while the actual data is at `response + 32`.
                    let returndata_size := mload(response)
                    revert(add(response, 32), returndata_size)
                }
            } else {
                revert PRBProxy_ExecutionReverted();
            }
        }

And the execution will revert here and it doesnt matter what data it returns, if there really was a silent revert then it would be a problem in the PRBProxy not in the telcoin contracts.

PRBProxy.execute() is not called with .call() so we dont need to check the return value because execution will revert there and the transaction will fail

[amshirif]

I agree with the escalation.

[nevillehuang]

In that case this issue should be invalid.

[Evert0x]

Planning to accept escalation and invalidate issue

[Evert0x]

Result:
Invalid
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xLogos: accepted

[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.