Automation Manual Review Findings

ANO-01M: Unconventional Self-Authorization

Type	Severity	Location
Logical Fault		Automation.sol:L147, L178, L221, L255, L290, L330, L361

Description:

The Automation contract's various functions will ultimately result in a self-call to the NonDelegateMulticall::multicall function which in turn will validate that its caller is part of the whitelisted callers in the registry of the system.

This effectively infers that the Automation contract itself will be set as a whitelisted caller which might not be the desired case as it would affect access control rules of several other contracts.

Impact:

The current self-call mechanism of executing batch multicall operations through the Automation contract is non-standard and inefficient.

Example:

Copy
258function compoundFor(
259    INftAutomation[] memory strategies,
260    Sickle[] memory sickles,
261    NftPosition[] memory positions,
262    NftCompound[] memory params,
263    bool[] memory inPlace,
264    address[][] memory sweepTokens
265) external onlyApprovedAutomator {
266    uint256 strategiesLength = strategies.length;
267    if (
268        strategiesLength != sickles.length
269            || strategiesLength != positions.length
270            || strategiesLength != params.length
271            || strategiesLength != sweepTokens.length
272    ) {
273        revert InvalidInputLength();
274    }
275
276    address[] memory targets = new address[](strategiesLength);
277    bytes[] memory data = new bytes[](strategiesLength);
278    for (uint256 i; i < strategiesLength; i++) {
279        Sickle sickle = sickles[i];
280        NftPosition memory position = positions[i];
281        targets[i] = address(strategies[i]);
282        data[i] = abi.encodeCall(
283            INftAutomation.compoundFor,
284            (sickle, position, params[i], inPlace[i], sweepTokens[i])
285        );
286        emit NftCompoundedFor(
287            sickle, address(position.nft), position.tokenId
288        );
289    }
290    this.multicall(targets, data);
291}

Recommendation:

We advise this trait to be re-evaluated, and the NonDelegateMulticall::multicall function to potentially permit a self-call to be made without authorization from the registry as it would infer the contract itself has applied proper sanitization to the call being made.

Alleviation (6ab7af3bb495b817ffec469255ea679b1813eecb):

The vfat team evaluated this exhibit and opted to retain the current behaviour in place as they consider the whitelisting of the Automation contract a requirement for other interactions within the system, such as invocations of the INFTautomation.compoundFor function signature.

As such, we consider this exhibit to have been safely acknowledged.

ANO-02M: Incompatible Implementation Types

Type	Severity	Location
Logical Fault		Automation.sol:L8, L84, L86-L92

Description:

The NonDelegateMulticall dependency of the Automation contract will inherit from the SickleStorage contract that is meant to be utilized as an upgradeable dependency whereas the Automation contract is meant to be an immutable contract deployment.

Impact:

The SickleStorage implementation appears to be redundant in the Automation implementation, so an informational severity is more adequate for it.

Example:

Copy
84address payable public approvedAutomator;
85
86constructor(
87    SickleRegistry registry_,
88    address payable approvedAutomator_,
89    address admin_
90) Admin(admin_) NonDelegateMulticall(registry_) {
91    approvedAutomator = approvedAutomator_;
92}

Recommendation:

We advise the NonDelegateMulticall contract to no longer inherit from the SickleStorage contract as it does not actively utilize it, or the Automation contract to be made upgradeability-compatible so as to prevent a mixture of non-upgradeable and upgradeable dependencies.

Alleviation (6ab7af3bb495b817ffec469255ea679b1813eecb):

The NonDelegateMulticall contract was updated to no longer inherit the SickleStorage implementation, addressing this issue as a side-effect.