hats-finance / Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb

0 stars 1 forks source link

Missing events for functions that change critical parameters #24

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf3d6f90041902c66d0913414c5f6f61f3314d024565eeb0c1ca60f33ae1812ac Severity: low

Description: Description\ The onlyOwner functions that change critical parameters should emit events.

1) Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

2) The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

3) Missing events do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.

In CvgCvxStakingPositionService.sol contract, below are owner functions that do not emit any events in the contracts.

    function setDepegPercentage(uint256 _depegPercentage) external onlyOwner {
        require(_depegPercentage >= 1000, "PERCENTAGE_TOO_LOW");
        depegPercentage = _depegPercentage;
    }

In CVX1.sol contract, below are owner functions that do not emit any events in the contracts.

    function setCvxRewardPool(ICvxRewardPool _cvxRewardPool) external onlyOwner {
        CVX.approve(address(cvxRewardPool), 0);
        CVX.approve(address(_cvxRewardPool), type(uint256).max);

        cvxRewardPool = _cvxRewardPool;
    }

In CvxConvergenceLocker.sol contract, below are owner functions that do not emit any events in the contracts.

    function setCvxDelegateRegistry(IDelegateRegistry delegateRegistry) external onlyOwner {
        cvxDelegateRegistry = delegateRegistry;
    }

    function setCvxStakingPositionService(address _cvxStakingPositionService) external onlyOwner {
        cvxStakingPositionService = _cvxStakingPositionService;
    }

    function setMintFees(uint256 _fees) external onlyOwner {
        /// @dev maximum allowed: 2%
        require(_fees <= 200, "FEES_TOO_BIG");
        mintFees = _fees;
    }

Recommendation to fix\ Add events to all onlyOwner functions that change critical parameters.

NOTE: Issue applicable to all such instances in inscope contracts

0xRizwan commented 6 months ago

Similar issue in CvxRewardDistributor.sol at L-263 and in CvxStakingPositionManager.sol at L-170--L-176 and in StakingServiceBase.sol at L-1040

0xR3vert commented 6 months ago

We don't consider the lack of event on Owner functions as a problem, it's completely assumed. In conclusion, this issue is invalid.

0xRizwan commented 5 months ago

@0xR3vert Based on contest rules for low severity,

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

I believe, the issue deserves low severity. Historically, missing events on critical functions are low severity issues on various audit platforms. The first judgement by lead auditor has also considered it low severity.