Watchers::verifyRemoveLiquidity is missing implementation logic

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/governance/Watchers.sol#L8
https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/helpers/BaseConnector.sol#L94

Vulnerability details

https://docs.noya.ai/technical-architecture/technical-architecture/roles-in-noya

The Watchers role is responsible to make sure the execution of NOYA is going on correctly. If there is any misbehaving (like price manipulation or any suspicious actions from the keepers) the watchers can undo the action.

In the current implementation of the BaseConnector::sendTokensToTrustedAddress there is a call to the Watchers contract and more specifically the verifyRemoveLiquidity function that takes in 3 arguments - amount, newAmount and newData.

function sendTokensToTrustedAddress(address token, uint256 amount, address caller, bytes memory data)
        external
        returns (uint256)
    {
        emit TransferTokensToTrustedAddress(token, amount, caller, data);

        (address accountingManager,) = registry.getVaultAddresses(vaultId);

        if (msg.sender == accountingManager) {
            (,,,, address watcherContract,) = registry.getGovernanceAddresses(vaultId);

            (uint256 newAmount, bytes memory newData) = abi.decode(data, (uint256, bytes));

@>          Watchers(watcherContract).verifyRemoveLiquidity(amount, newAmount, newData);

            IERC20(token).safeTransfer(address(accountingManager), newAmount);
            amount = newAmount;
        } else if (registry.isAnActiveConnector(vaultId, msg.sender) || msg.sender == registry.flashLoan()) {
            IERC20(token).safeTransfer(address(msg.sender), amount);
        } else {
            uint256 routeId = abi.decode(data, (uint256));
            swapHandler.verifyRoute(routeId, msg.sender);
            if (caller != address(this)) revert IConnector_InvalidAddress(caller);
            IERC20(token).safeTransfer(msg.sender, amount);
        }

        _updateTokenInRegistry(token);

        return amount;
    }

However, inside the Watchers::verifyRemoveLiquidty , there is no implementation logic and nothing is being done with the passed arguments from the call made in BaseConnector::sendTokensToTrustedAddress.

pragma solidity 0.8.20;

import "./Keepers.sol";

contract Watchers is Keepers {
    constructor(address[] memory _owners, uint8 _threshold) Keepers(_owners, _threshold) { }
    function verifyRemoveLiquidity(uint256 withdrawAmount, uint256 sentAmount, bytes memory data) external view { }
}

Impact

The Watchers contract is missing its implementation logic, effectively leaving the protocol without a functioning Watchers role. This role is crucial for ensuring the integrity of strategy execution and for monitoring any misbehavior from the keepers.

Proof of Concept

https://github.com/code-423n4/2024-04-noya/blob/main/contracts/governance/Watchers.sol#L8

Tools Used

Manual Review

Recommended Mitigation Steps

Add the missing implementation logic to the Watchers::verifyRemoveLiquidity function to ensure that the Watchers contract has the necessary logic to fulfill its role.

Assessed type

Invalid Validation

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #1520

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[PetarTolev]

Hi @gzeon-c4,

I would like to request a reconsideration of the severity for issue #1434. Although initially marked as QA, I believe it better aligns with the criteria for a Medium severity classification outlined in the Code4rena Docs:
2 — Med: Although assets aren't directly at risk, the protocol's functionality or availability could be impacted, potentially leading to value leaks via a hypothetical attack path under certain assumptions and external requirements.

Given that the Watchers are a critical component of Noya's security, as outlined in the Noya Security Measures, this issue significantly affects the protocol's governance and operational integrity.

The absence of implementation in the Watchers::verifyRemoveLiquidity function means there’s no validation of actions that should be monitored by the Watchers. This directly affects the protocol's ability to prevent misbehavior and consequently undermines the effectiveness of its security measures.

The missing logic creates a potential attack path where Keepers could exploit the lack of oversight, leading to unauthorized actions with severe and unpredictable consequences.

Although there is no immediate proof of attack, the lack of oversight presents a clear risk that needs to be addressed. Therefore, this issue fits the description of Medium severity, because it impacts the protocol's functionality and could lead to value leaks under certain conditions and external requirements (e.g., a Keeper misbehaving).