search

sherlock-audit / 2024-01-rio-vesting-escrow-judging

3 stars 2 forks source link

recursiveEth - Ensuring Distinct Roles Owner and Manager Addresses Should Not Be the Same, as it could lead to confusion or unintended consequences. #107

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 9 months ago

recursiveEth

medium

Ensuring Distinct Roles Owner and Manager Addresses Should Not Be the Same, as it could lead to confusion or unintended consequences.

Summary

The code does not explicitly check whether the manager and owner addresses are the same in VestingEscrowFactory.sol:Constructor(). There's a potential vulnerability if the manager and owner addresses are allowed to be the same, as it could lead to confusion or unintended **consequences.

Vulnerability Detail

If the manager and owner addresses are allowed to be the same, it may lead to potential security risks or operational issues.Having the manager and owner as distinct entities is a common security practice to separate responsibilities and prevent unauthorized actions.

Impact

Having the similar owner and manager could risk some of the operations of the vestingEscrow contract, as owner have more rights and more load of operations compare to manager like VestingEscrow.sol:revokeAll(), VestingEscrow.sol:revokeUnvested() and VestingEscrow.sol:permanentlyDisableFullRevocation() are sensitive functions which can significantly impact the functioning of the vestingEscrow contract. By having same address as owner he manager can control these functions. So if manager turns out he can harm the protocol and can have unintended consequences.

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L30

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();

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

        _transferOwnership(_owner);
    }

Tool used

Manual Review

Recommendation

Ensure Manager and Owner Addresses are Distinct, Implement a check to ensure that the manager and owner addresses are not the same. This helps maintain a clear separation of roles and reduces the risk of unauthorized actions.

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();
+       require(_owner != _manager,"Revert Reason: owner and manager having similar addresses");
        vestingEscrowImpl = _vestingEscrowImpl;
        token = _token;
        manager = _manager;
        votingAdaptor = _votingAdaptor;

        _transferOwnership(_owner);
    }
nevillehuang commented 9 months ago

Invalid, this would constitute admin input error when deploying not valid based on sherlock rules

  1. User input validation: User input validation to prevent user mistakes is not considered a valid issue.
sherlock-admin2 commented 9 months ago

1 comment(s) were left on this issue during the judging contest.

pratraut commented:

'invalid as warden have not demonstrated impactful scenario'