eta - Access Control Misconfiguration in `VestingEscrow::revokeUnvested`

[sherlock-admin]

eta

high

Access Control Misconfiguration in VestingEscrow::revokeUnvested

Summary

To ensure the correct implementation of revokeUnvested function and prevent unintended token vesting and customer loss, it's crucial to modify the _checkOwnerOrManager function to permit execution by either the owner or manager independently, ensuring flexibility and mitigating potential negative impacts.

Vulnerability Detail

In the VestingEscrow contract, the onlyOwnerOrManager modifier enforces this restriction as its name, but its internal _checkOwnerOrManager function incorrectly blocks execution if both owner and manager are different entities.

The revokeUnvested function intended to revoke unvested tokens and disable further flow. However, it can only be executed by msg.sender who are both the contract's owner and manager due to the onlyOwnerOrManager modifier. If owner and manager are distinct individuals, neither can independently call revokeUnvested function, hindering the intended functionality.

Moreover,If owners or managers find that they are unable to revoke unvested tokens, this could cause market panic. Owners may then call the contract's revokeAll function to overreact and revoke all locked and unclaimed funds. This could lead our company to lose a significant numbers of customer, impacting not only business but also reputation.

Therefore, it is recommended to modify _checkOwnerOrManager function’s logic to allow execution if msg.sender is either the owner OR the manager, not requiring both conditions to be true simultaneously.

VestingEscrow::_checkOwnerOrManager

    /// @dev Throws if called by any account other than the owner or manager.
    function _checkOwnerOrManager() internal view {
-       if (msg.sender != _owner() && msg.sender != _manager()) {
+       if (msg.sender != _owner() | | msg.sender != _manager()) {
            revert NOT_OWNER_OR_MANAGER(msg.sender);
        }
    }

VestingEscrow::onlyOwnerOrManager

    /// @notice Throws if called by any account other than the owner or manager.
    modifier onlyOwnerOrManager() {
        _checkOwnerOrManager();
        _;
    }

VestingEscrow::revokeUnvested, revokeAll

    /// @notice Disable further flow of tokens and revoke the unvested part to owner.
    function revokeUnvested() external onlyOwnerOrManager {
        uint256 revokable = locked();
        if (revokable == 0) revert NOTHING_TO_REVOKE();

        disabledAt = uint40(block.timestamp);

        token().safeTransfer(_owner(), revokable);
        emit UnvestedTokensRevoked(msg.sender, revokable);
    }

    /// @notice Disable further flow of tokens and revoke all tokens to owner.
    function revokeAll() external onlyOwner {
        if (!isFullyRevokable) revert NOT_FULLY_REVOKABLE();
        if (isFullyRevoked) revert ALREADY_FULLY_REVOKED();

        uint256 revokable = locked() + unclaimed();
        if (revokable == 0) revert NOTHING_TO_REVOKE();

        isFullyRevoked = true;
        disabledAt = uint40(block.timestamp);

        token().safeTransfer(_owner(), revokable);
        emit VestingFullyRevoked(msg.sender, revokable);
    }

Impact

Revoke Function Inaccessibility: The inability to call VestingEscrow::revokeUnvested under these conditions prevents revoking unvested tokens or disabling token flow when needed, potentially leading to unintended token vesting beyond intended periods, inability to address unforeseen circumstances requiring revocation, and potential financial and contractual implications.

Code Snippet

VestingEscrow::_checkOwnerOrManager

VestingEscrow::onlyOwnerOrManager

VestingEscrow::revokeUnvested, revokeAll

Tool used

Manual Review

Recommendation

Modify _checkOwnerOrManage: Restructure the function's logic to allow execution if msg.sender is either the owner OR the manager, not requiring both conditions to be true simultaneously. This ensures either authorized entity can independently call revokeUnvested.

Duplicate of #75

[sherlock-admin2]

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

pratraut commented:

'invalid due to check is already correct'