code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L157

Vulnerability details

Impact

The base ARCDVestingVault contract has a Manager account attached to it, that can perform privileged actions. One of said actions, is the revokeGrant, which can revoke the grant from a user, and it's voting power. Since there is no timelock on the revoke function, the manager account can use this to manipulate the voting process, observing the mempool and frontrunning the votes that they don't "like", coming from grants, with a revoke call. Although this function is administrative, in a decentralized environment, is a duty of the protocol to protect the users and members of the DAO from centralization issues and rouge administrative accounts, especially in a Governance system.

Also there is no indication that the manager will be a timelock account. Which is usually highlighted in, comments, documentations and naming of the variables, in all other istances of the protocol, even within the same contract, eg:

 * @title ARCDVestingVault
 * @author Non-Fungible Technologies, Inc.
 *
 * This contract is a vesting vault for the Arcade token. It allows for the creation of grants
 * which can be vested over time. The vault has a manager who can add and remove grants.
 * The vault also has a timelock which can change the manager.

And

    // ========================================= CONSTRUCTOR ============================================

    /**
     * @notice Deploys a new vesting vault, setting relevant immutable variables
     *         and granting management power to a defined address.
     *
     * @param _token              The ERC20 token to grant.
     * @param _stale              Stale block used for voting power calculations
     * @param manager_            The address of the manager.
     * @param timelock_           The address of the timelock.
     */
    constructor(IERC20 _token, uint256 _stale, address manager_, address timelock_) BaseVotingVault(_token, _stale) {
        if (manager_ == address(0)) revert AVV_ZeroAddress("manager");
        if (timelock_ == address(0)) revert AVV_ZeroAddress("timelock");

        Storage.set(Storage.addressPtr("manager"), manager_);
        Storage.set(Storage.addressPtr("timelock"), timelock_);
        Storage.set(Storage.uint256Ptr("entered"), 1);
    }

Proof of Concept

The revokeGrant(address who) function of the ARCDVestingVault contract (https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L157), will withdraw, both to the user and to the manager, all the tokens locked in a grant and then it updates the voting power of the targeted account (to 0):

        // get the amount of withdrawable tokens
        uint256 withdrawable = _getWithdrawableAmount(grant);
        grant.withdrawn += uint128(withdrawable);
        token.safeTransfer(who, withdrawable);

        // transfer the remaining tokens to the vesting manager
        uint256 remaining = grant.allocation - grant.withdrawn;
        grant.withdrawn += uint128(remaining);
        token.safeTransfer(msg.sender, remaining);

        // update the delegatee's voting power
        _syncVotingPower(who, grant);

By setting grant.withdrawn to be equal to grant.allocation, the _syncVotingPower function (https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L341) will cause the new voting power, of the grant, to be 0:

        uint256 delegateeVotes = votingPower.loadTop(grant.delegatee);

        uint256 newVotingPower = grant.allocation - grant.withdrawn;

The lack of timing restriction of said action will allow the manger to front run any vote, originated form a grant, with a revokeGrant call to prevent it from happening, manipulating the vote.

Scenario: 1: Manager gives a grant of 10 votes to User 2: User vote for a proposal that goes against the interest of Manager 3: Manager observes the mempool, sees that User is voting against the manager's interest, and front run the vote with a revoke call 4: The voting power of User is set to 0, decreasing the chances that the proposal will pass

Recommended Mitigation Steps

Using a timelock account as the manager will reduce centralization and solve the issue, but would make other safe functions unnecessarily slower. A adding timelock specifically to the revokeGrant function, will still solve the issue, without introducing new problems.

Assessed type

Governance

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #555

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Out of scope

code-423n4 / 2023-07-arcade-findings