VTVL contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L129 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L381

Vulnerability details

A user can withdraw the currently claimable balance of their tokens at any time by calling the VTVLVesting.withdraw() function. This function will withdraw the user's currently claimable balance and update the amountWithdrawn field of the user's claim to reflect the new amount withdrawn.

However, if the claim is fully vested and all tokens have been withdrawn, it is impossible to create a new claim for this user. This is because the VTVLVesting._createClaimUnchecked() function requires that the user does not already have a claim, which is enforced by the hasNoClaim() modifier.

Impact

A user can only ever have a single, fully vested claim. It is not possible to create a follow-up claim for the same user address.

Proof of Concept

VTVLVesting.sol#L381

/**
@notice Withdraw the full claimable balance.
@dev hasActiveClaim throws off anyone without a claim.
  */
function withdraw() hasActiveClaim(_msgSender()) external {
    // Get the message sender claim - if any

    Claim storage usrClaim = claims[_msgSender()];

    // we can use block.timestamp directly here as reference TS, as the function itself will make sure to cap it to endTimestamp
    // Conversion of timestamp to uint40 should be safe since 48 bit allows for a lot of years.
    uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));

    // Make sure we didn't already withdraw more that we're allowed.
    require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

    // Calculate how much can we withdraw (equivalent to the above inequality)
    uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

    // "Double-entry bookkeeping"
    // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens.
    usrClaim.amountWithdrawn += amountRemaining;
    // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced
    numTokensReservedForVesting -= amountRemaining;

    // After the "books" are set, transfer the tokens
    // Reentrancy note - internal vars have been changed by now
    // Also following Checks-effects-interactions pattern
    tokenAddress.safeTransfer(_msgSender(), amountRemaining);

    // Let withdrawal known to everyone.
    emit Claimed(_msgSender(), amountRemaining);
}

VTVLVesting.sol#L129

/**
@notice Modifier which is opposite hasActiveClaim
@dev Requires that all fields are unset
*/
modifier hasNoClaim(address _recipient) {
    Claim storage _claim = claims[_recipient];
    // Start timestamp != 0 is a sufficient condition for a claim to exist
    // This is because we only ever add claims (or modify startTs) in the createClaim function
    // Which requires that its input startTimestamp be nonzero
    // So therefore, a zero value for this indicates the claim does not exist.
    require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");

    // We don't even need to check for active to be unset, since this function only
    // determines that a claim hasn't been set
    // require(_claim.isActive == false, "CLAIM_ALREADY_EXISTS");

    // Further checks aren't necessary (to save gas), as they're done at creation time (createClaim)
    // require(_claim.endTimestamp == 0, "CLAIM_ALREADY_EXISTS");
    // require(_claim.linearVestAmount + _claim.cliffAmount == 0, "CLAIM_ALREADY_EXISTS");
    // require(_claim.amountWithdrawn == 0, "CLAIM_ALREADY_EXISTS");
    _;
}

Tools Used

Manual review

Recommended mitigation steps

When withdrawing an active claim by calling the VTVLVesting.withdraw function, consider checking if the claim is fully vested and if so, set usrClaim.isActive to false as well as usrClaim.startTimestamp to 0.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L450

Vulnerability details

The VTVLVesting contract implements the withdrawOtherToken function which allows admins to withdraw any (accidentally) received ERC-20 tokens from the contract. However, this function does not work for ERC-721 tokens due to the transfer function (called by safeTransfer) not being implemented for ERC-721 tokens.

Impact

Accidentally received ERC-721 tokens are trapped within the VTVLVesting contract.

Proof of Concept

VTVLVesting.sol#L450

function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin {
    require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
    uint256 bal = _otherTokenAddress.balanceOf(address(this));
    require(bal > 0, "INSUFFICIENT_BALANCE");
    _otherTokenAddress.safeTransfer(_msgSender(), bal);
}

Tools Used

Manual review

Recommended mitigation steps

Consider implementing a separate sweeping function for ERC-721 tokens.