code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ARCDVestingVault.sol#L122 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ARCDVestingVault.sol#L228-L253

Vulnerability details

Impact

The ImmutableVestingVault contract does not support sending multiple grants to the same recipient, which is functionality that should be supported. This is because the addGrantAndDelegate function will revert if grant.allocation != 0 for a given user. However, grant.allocation is never set to 0, even when the recipient has claimed all their tokens from the grant (by calling claim). This is improper behavior and will break all logic which relies on granting the same recipient address multiple grants.

Proof of Concept

The ImmutableVestingVault contract inherits almost all functionality from the ARCDVestingVault contract, with the exception that the revokeGrant function will revert on all calls. This is an issue because the revokeGrant function is the only place in which grant.allocation is set to 0.

This in turn is an issue because the addGrantAndDelegate function, which is used to create a grant for a recipient, has the following check:

if (grant.allocation != 0) revert AVV_HasGrant();

There is also no other place in which grant.allocation is set to 0, even if the recipient has claimed all the tokens from the grant. For example, the claim function is defined as follows:

function claim(uint256 amount) external override nonReentrant {
    if (amount == 0) revert AVV_InvalidAmount();

    // load the grant
    ARCDVestingVaultStorage.Grant storage grant = _grants()[msg.sender];
    if (grant.allocation == 0) revert AVV_NoGrantSet();
    if (grant.cliff > block.number) revert AVV_CliffNotReached(grant.cliff);

    // get the withdrawable amount
    uint256 withdrawable = _getWithdrawableAmount(grant);
    if (amount > withdrawable) revert AVV_InsufficientBalance(withdrawable);

    // update the grant's withdrawn amount
    if (amount == withdrawable) {
        grant.withdrawn += uint128(withdrawable);
    } else {
        grant.withdrawn += uint128(amount);
        withdrawable = amount;
    }

    // update the user's voting power
    _syncVotingPower(msg.sender, grant);

    // transfer the available amount
    token.safeTransfer(msg.sender, withdrawable);
}

Even when a user has withdrawn all of their tokens from the grant (grant.withdrawn == grant.allocation), the grant will never be deleted (grant.allocation = 0). Thus, a recipient can never get more than a single grant, which is invalid behavior.

Tools Used

Manual review

Recommended Mitigation Steps

In the claim function of the ARCDVestingVault contract, when the recipient has withdrawn all tokens from the grant, the grant should be deleted similar to how it is done at the bottom of the revokeGrant function of the ARCDVestingVault contract.

Assessed type

Other

141345 commented 1 year ago

QA might be more appropriate.

c4-sponsor commented 1 year ago

PowVT marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

PowVT marked the issue as disagree with severity

PowVT commented 1 year ago

QA, duplicate of #240 . Not intended to be re-used for a user and there is a easy workaround if needed.

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)