6.1 ETH can never be rescued from ClaggRescueAdapter
The ClaggRescueAdapter contract includes a rescue function designed to allow recovery of tokens from the contract after they have been whitelisted. The function aims to support rescuing both ERC20 tokens and ETH:
function rescue(address token, uint256 amount) external onlyOperatorOrOwner {
1==> if (token == address(0)) revert Errors.INVALID_TOKEN();
if (!MainStorageLib.rescueTokenExists(token)) revert Errors.INVALID_RESCUE_TOKEN();
2==> if (token == address(0)) {
(bool success,) = payable(msg.sender).call{value: amount}("");
if (!success) revert Errors.ETH_TRANSFER_FAILED();
} else {
IERC20(token).safeTransfer(msg.sender, amount);
}
}
There is a contradictory logic in the implementation:
- First (1), the function rejects a zero address token and immediately reverts with
INVALID_TOKEN error;
- However, later (2), it attempts to handle the case where
token == address(0) by sending ETH to the caller;
Due to the initial check, the ETH rescue code is unreachable, effectively preventing ETH from ever being rescued from the contract.
Recommendation
Consider removing or modifying the initial zero address check (highlighted at point 1) to allow the function to handle ETH rescue correctly
6.2 Storage collision between ClaggRescueAdapter and ClaggMainAdapter
The ClaggMain contract acts as the protocol’s entry point and delegates all calls to appropriate adapter contracts via a fallback function:
fallback() external payable {
uint256 dataLength = msg.data.length - 20;
address adapter = address(bytes20(msg.data[dataLength:]));
if (!MainStorageLib.adapterExists(adapter)) revert Errors.INVALID_ADAPTER();
bytes calldata data = msg.data[:dataLength];
==> (bool success, bytes memory result) = adapter.delegatecall(data);
if (success) {
assembly {
return(add(result, 32), mload(result))
}
}
assembly {
revert(add(result, 32), mload(result))
}
}
Because delegatecall is used, the adapter contracts execute in the context of the ClaggMain contract, meaning they share the same storage space.
Both ClaggRescueAdapter and ClaggMainAdapter declare a state variable named timelock at the same storage slot (slot 0):
contract ClaggMainAdapter is EnforceOwnership {
event MainTimelockSet(uint96 timelock);
uint96 public timelock;
// --SNIP
}
contract ClaggRescueAdapter is EnforceOwnership {
using SafeERC20 for IERC20;
event RescueTimelockSet(uint96 timelock);
uint96 public timelock;
// --SNIP
}
Due to the shared storage layout during delegatecall execution, updating timelock in one adapter will unintentionally overwrite the timelock value in the other.
Recommendation
From natspec, it seems the timelock should be set once, consider making them immutable so they will be encoded in the bytecode directly.
6.3 Clagg4626BaseAdapter::getStakeLimit always returns incorrect value
The getStakeLimit function is designed to return the remaining deposit capacity on the underlying ERC4626 vault (pool). The implementation calls the vault’s maxDeposit method as follows:
function getStakeLimit(address pool) external view override returns (uint256) {
PoolConfig storage poolConfig = _getPoolConfig(pool);
address token = poolConfig.token;
return IERC4626(pool).maxDeposit(token);
}
However, the maxDeposit function in the ERC4626 standard expects the receiver’s address as its parameter, representing the account intending to deposit tokens, not the underlying asset’s address. Passing the token address instead leads to incorrect return values from maxDeposit, causing getStakeLimit to report an inaccurate remaining deposit limit in all cases, depending on the pool implementation.
Recommendation
Since ClaggMain is the contract making deposits on behalf of users, the address to be passed to maxDeposit should be the contract's address instead.
6.4 Initialization of path variable is redundant for IUniswapV3Router::exactInputSingle
The _swapFrom_UNISWAP_V3 function in the Swapper contract handles token swaps using UniswapV3. It decodes SwapData to get specific UniswapV3 parameters, including an intermediateToken.
When a swap request does not involve an intermediate token (i.e., uniswapV3SwapData.intermediateToken == address(0)), the function proceeds with a direct swap using exactInputSingle. In this scenario, the path variable is initialized with the encoded data received. However, the exactInputSingle function does not utilize the path variable.
function _swapFrom_UNISWAP_V3(address _from, address _to, uint256 _amountIn, SwapData memory _swapData)
internal
returns (uint256 _amountOut)
{
UniswapV3SwapData memory uniswapV3SwapData = abi.decode(_swapData.data, (UniswapV3SwapData));
bytes memory path;
if (uniswapV3SwapData.intermediateToken == address(0)) {
// @audit-issue This path variable is initialized but never used in the exactInputSingle call.
path = abi.encodePacked(_from, uniswapV3SwapData.fee, _to);
_amountOut = IUniswapV3Router(_swapData.router).exactInputSingle(
IUniswapV3Router.ExactInputSingleParams({
tokenIn: _from,
tokenOut: _to,
fee: uniswapV3SwapData.fee,
recipient: address(this),
amountIn: _amountIn,
amountOutMinimum: 0,
sqrtPriceLimitX96: 0
})
);
} else {
// ...
}
}
There is no impact on the functioning of the _swapFrom_UNISWAP_V3 function. Initializing the path variable in this specific condition is unnecessary as the exactInputSingle function does not use it. This redundancy does not impact the function's operation but represents unused code/variable.
Recommendation
Remove the path initialization.
