One Admin can remove all others and run the contract on its own

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L33-L43

Vulnerability details

As all _admins are equal, any of them turning malicious can remove all others in one transaction.

As an example, one admin is a system, that have a bug that Alice exploits to trick that system to call setAdmin(Alice, true). Alice back-runs that transaction, removing all other admins and use adminOnly revokeClaim() and withdraw*() functions to remove any obtainable funds from VTVLVesting contract.

Proof of Concept

As another example, one admin, Bob, can legitimately remove another, Mike.

Mike, observing that in mempool, can front run the call, removing Bob instead.

That is possible as any admin can remove all the others:

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L33-L43

    /**
     * @notice Set/unset Admin Access for a given address.
     *
     * @param admin - Address of the new admin (or the one to be removed)
     * @param isEnabled - Enable/Disable Admin Access
     */
    function setAdmin(address admin, bool isEnabled) public onlyAdmin {
        require(admin != address(0), "INVALID_ADDRESS");
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }

And they are equal access wise:

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L24-L27

    modifier onlyAdmin() {
        require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
        _;
    }

Recommended Mitigation Steps

Consider adding introduce head admin role, the only one who can add and remove simple admins, who can run the vestings.

[0xean]

closing as invalid, per README admins are trusted and the sponsor called all of the "malicious admin" stuff out of scope.