eta - Missing Validation in `VestingEscrowFactory::constructor`, Optimize Setter Function In `VestingEscrowFactory` contract and Use Ownable2Step in the `OZVotingAdaptor` contract

[sherlock-admin]

eta

medium

Missing Validation in VestingEscrowFactory::constructor, Optimize Setter Function In VestingEscrowFactory contract and Use Ownable2Step in the OZVotingAdaptor contract

Summary

This code review highlights three problems for improvement: 1)Missing Validation in VestingEscrowFactory::constructor: The constructor doesn't validate if _manager is address(0), which represents null or no address. This means the contract could be initialized with a non-existent manager address. 2)Optimize Setter Function In VestingEscrowFactory contract: To enhance change tracking and memory efficiency by directly recording old and new values of critical parameters within updateVotingAdaptor and changeManager, eliminating the need for redundant caching. 3)Use Ownable2Step instead of Ownable in the OZVotingAdaptor contract

Vulnerability Detail

1.Missing Validation in VestingEscrowFactory::constructor

The constructor doesn't validate if _manager is address(0), which represents null or no address. If a function calls or interacts with the manager address without prior checks, it could fail or behave unexpectedly when the address is null.

A malicious actor might exploit this vulnerability by intentionally setting _manager to address(0) to bypass intended access controls or disrupt contract operations. Contract functionality that relies on a valid manager might not execute as planned, leading to unpredictable outcomes or errors.

Therefore, it is recommended to add a check to ensure _manager is not address(0) as shown below:

VestingEscrowFactory::constructor

    constructor(address _vestingEscrowImpl, address _token, address _owner, address _manager, address _votingAdaptor) {
        if (_vestingEscrowImpl == address(0)) revert INVALID_VESTING_ESCROW_IMPL();
        if (_token == address(0)) revert INVALID_TOKEN();
        if (_owner == address(0)) revert INVALID_OWNER();

+       if (_manager == address(0)) revert INVALID_MANAGER();

        vestingEscrowImpl = _vestingEscrowImpl;
        token = _token;
        manager = _manager;
        votingAdaptor = _votingAdaptor;

        _transferOwnership(_owner);
}

2.Improve Logic in Setter Functions of updateVotingAdaptor and changeManager in VestingEscrowFactory contract

Record both old and new values: Directly within setter functions, it is recommended to record both its previous and updated values of critical parameters for both updateVotingAdaptor and changeManager functions. This enables accurate tracking of modifications, informed analysis of patterns, and the potential to revert to prior states if needed.

Eliminate unnecessary caching: If the old value solely serves immediate actions (e.g., event emission), access it directly within the setter. This streamlines code, potentially reduces memory overhead, and enhances readability.

VestingEscrowFactory::updateVotingAdaptor

    /// @notice Change the voting adaptor of the vesting contracts.
function updateVotingAdaptor(address _votingAdaptor) external onlyOwner {

+       emit VotingAdaptorUpgraded(votingAdaptor, _votingAdaptor);

        votingAdaptor = _votingAdaptor;
-       emit VotingAdaptorUpgraded(_votingAdaptor);
}

VestingEscrowFactory::changeManager

    /// @notice Change the manager of the vesting contracts.
function changeManager(address _manager) external onlyOwner {

+       emit ManagerChanged(manager, _manager);

        manager = _manager;
-       emit ManagerChanged(_manager);
}

3. Use Ownable2Step instead of Ownable in the OZVotingAdaptor contract

Ownable2Step and Ownable2StepUpgradeable safeguard against accidental ownership transfers to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own. This proactive approach mitigates errors like typos and ensures only prepared addresses assume ownership, enhancing contract security and integrity.

OZVotingAdaptor::Ownable

-  contract OZVotingAdaptor is IVotingAdaptor, Ownable {

+  contract OZVotingAdaptor is IVotingAdaptor, Ownable2Step {

Impact

1)Missing Validation in VestingEscrowFactory::constructor: Failing to validate the _manager address in the constructor could cause unexpected failures, potential security breaches, and unpredictable contract behavior. This is because functions interacting with a null manager address might not execute as intended, exposing vulnerabilities for exploitation by malicious actors and disrupting expected contract operations.

2)Inaccurate Tracing and Analysis: Without capturing both old and new values in the updateVotingAdaptor and changeManager and functions, precise tracking of parameter changesis impossible, hindering trend analysis and issue diagnosis.

3)Use Ownable2Step instead of Ownable: Ownable2Step and Ownable2StepUpgradeable fortify contract ownership transfers by mandating explicit recipient acceptance, averting accidental transfers to incompatible addresses and ensuring only prepared addresses assume control.

Code Snippet

1)VestingEscrowFactory::constructor

2)VestingEscrowFactory::updateVotingAdaptor

VestingEscrowFactory::changeManager

3)OZVotingAdaptor::Ownable

Tool used

Manual Review

Recommendation

1)Implement Validation in VestingEscrowFactory::constructor:: Add a check to ensure _manager is not address(0) to prevent unexpected behavior or vulnerabilities.

2)Prioritize Change Tracking and Memory Efficiency: Capture both old and new values directly within setter functions of updateVotingAdaptor and changeManager for critical parameters to establish a detailed change history. When the old value is only required for immediate actions within the function, directly access it instead of caching, which streamlines code, potentially reduces memory overhead, and improves code readability.

3)Use Ownable2Step instead of Ownable in the OZVotingAdaptor contract

Duplicate of #109