kenzo - Timelocker can not execute transactions that send ETH value

[sherlock-admin]

kenzo

medium

Timelocker can not execute transactions that send ETH value

Summary

When forwarding transactions, Timelocker tries to forward the number of ETH specified by the tx issuer, but the execute function is not payable.
So the Timelocker can not actually be used to execute transactions that need msg.value.

Vulnerability Detail

When executing a queued transaction, the timelocker sends the number of ETH specified by the user - _value.

        (bool ok, bytes memory res) = _target.call{value: _value}(data);

However, the execute function is not payable.

    function execute(
        address _target,
        uint256 _value,
        string calldata _func,
        bytes calldata _data,
        uint256 _timestamp
    ) external onlyOwner returns (bytes memory) {

Therefore, the user specified _value must always be 0, and transactions that need a transfer of ETH can not be executed at all.

Impact

Timelocker is not robust and can not execute transactions that need the usage of msg.value.
While at the moment there are no such transactions that are needed,
the timelocker implementation can not be changed once set (eg. in the vault factory),
and you'll probably want to use only one timelocker for all the protocol,
so I believe it's worth bringing this to your attention.

Code Snippet

We can see that execute is not payable, yet tries to forward _value to the call.

    function execute(
        address _target,
        uint256 _value,
        string calldata _func,
        bytes calldata _data,
        uint256 _timestamp
    ) external onlyOwner returns (bytes memory) {
        ...
        (bool ok, bytes memory res) = _target.call{value: _value}(data);

Also note that there is no receive or fallback function, so Timelock can not receive ETH.

Tool used

Manual Review

Recommendation

Add payable to execute.

Duplicate of #387