VTVL contest - dic0de's results

Lines of code

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

Vulnerability details

Impact

The AccessProtected contract is used to add and govern the admin access control of the VTVLVesting contract. The deployer of the contract becomes the admin and has the ability of adding other admins via the setAdmin () function https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39-L43. The setAdmin () function allows admins to create as well as revoke admins. As such, an admin can revoke the deployer of the contract who is as well the contract owner the admin privileges ultimately incapacitating the deployer.

From the contract documentation, it was not envisioned that the deployer admin rights might be revoked as seen here via this statement:

This group initially starts with just having the contract deployer as the owner/admin. They can then add (or remove) other users as administrators, relegating them the same rights.

Moreover, the test cases did not cover this particular edge case and only tested the owner who is the deployer of the contract abilities on creating and revoking other admins.

Proof of Concept

On the VTVLVesting.js test file, add the following test code under Contract Creation:

  it('allows the owner to set and unset other user as an admin and the owner can be unset as well', async function() {
    const [owner, otherOwner] = await ethers.getSigners();

    const contract = await deployVestingContract();

    // Initially the other owner is not admin
    expect(await contract.isAdmin(otherOwner.address)).to.equal(false);

    // They'll become one after we set them as admin
    await (await contract.setAdmin(otherOwner.address, true)).wait();
    expect(await contract.isAdmin(otherOwner.address)).to.equal(true);
    
    // And then they'll stop being one right after we unset them
    await (await contract.setAdmin(owner.address, false)).wait();
    expect(await contract.isAdmin(owner.address)).to.equal(false);
  });

The test will run successfully as the owner will be unset as the admin

Tools Used

Recommended Mitigation Steps