Hot Signer Can Pass Arbitrary Data to Partially Whitelisted Functions

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L1028-L1155
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L1189-L1243
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L708-L726
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L728-L755

Vulnerability details

Description

The addCalldataCheck() and removeCalldataCheck() functions allow the timelock to add and remove calldatas for different index ranges of functions.

However, this mechanism contains a vulnerability where hot signers can exploit non-whitelisted parameters in function calls.

For example, if the timelock adds calldata for specific index range of a function, covering only the first parameter of that function, the hot signer can send arbitrary data for the remaining parameters even if this is not how safe owners what planned to do.

Similarly, if timelock removes index range and calldatas of a function that covers specific parameter, hot signer can send arbitrary data for that parameter, once index range covering that parameter is removed.

Impact

when timelock adds calldata or removes specific index range of a function, Hot signers gain ability to execute those functions with passing arbitrary values for parameters that do not have any coverage.

Proof of Concept

1. Add Calldata Example:

   the addCalldataCheck() function, whitelists multiple calldatas for single index range.

lets imagine we have someFunc(address, uint256) and timelock wants to whitelist both parameters, with addCalldataCheck() function.

to do so:

• to whitelist address parameter: the startIndex == 4 and endIndex == 36, and calldata is 0x4838B106FCe9647Bdf1E7877BF73cE8B0BAD5f97.
• to whitelist uint256 parameter: the startIndex == 37 and endIndex == 68, and calldata is 0xde0b6b3a7640000(1e18 in decimal).

now keep in mind the addCalldataCheck() function, whitelists multiple calldatas for single index range meaning a single parameter (it can cover all parameters in a single index range, but that's not how it's recommended to be used on #L1102-L1108, since it reduces flexibility), that means safe is going to schedule() two operations for Timelock, to be able to whitelist both of this calldatas for that function.

now lets imagine safe did schedule operation for both of those calldatas, and now it's ready for execution. hot signer chooses to execute an operation that is responsible for whitelisting address parameter of someFunc(address, uint256).

once this calldata is added for that function, hot signer ignores second operation and doesn't execute it (the operation that is responsible for whitelisting second parameter of someFunc(address, uint256)), and then executes the someFunc(address, uint256) function with passing arbitrary uint256.

2. Remove Calldata Example:

   lets imagine timelock has added calldatas for each paramter of someFunc(address, uint256) (keep in mind each parameter has different index range). now timelock decides to remove entire Index struct that covers first parameter with removeCalldataCheck() function.

after removal, this means hot signer can execute someFunc(address, uint256) with passing arbitrary address for first parameter, when calling this function.

Recommended Mitigation

maybe this is a design choice where, if certain parameters of a function, dont have restrictions set, it means hot signer can pass any arbitrary value for that parameter, but it's better to add a mapping inside the Timelock that enables/disables hot signer access to a function, even if calldatas are set for it.

with use of that mapping, all functions by default are disabled.

after Timelock adds a calldata for specific function, the safe owners review added calldatas and parts they haven't put any restrictions on, once reviewed, they decide to either cover new/same index range of that function by adding new calldatas or enable that function for hot signers use.

same can be applied when timelock wants to remove an entire index range from a function. first timelock disables the function and hot signers cannot call that function, until is re-enabled again. in the meantime, timelock removes an index range and safe owners review the parts they put restrictions and parts they haven't, and then they either decide to add/remove calldata for/from that function or re-enable that function so hot signers can use it again but with new restrictions.

Assessed type

Other

[ElliotFriedman]

this is expected behavior, so this should be marked as invalid.

[ElliotFriedman]

We should not be paying out for this, this is part of the system design.

[GalloDaSballo]

This is the design of the system

The design is as follows:

• A calldata has no checks -> It's no valid
• A calldata has wildcard -> Accepts anything
• A calldata has specific checks -> Ensures the check is valid on that part of calldata, wildcard the rest

I believe this is invalid

[c4-judge]

GalloDaSballo marked the issue as nullified