FairSide contest - cmichel's results

Handle

cmichel

Vulnerability details

The FSDVesting.updateVestedTokens function is supposed to be called by the FDS contract only (which also mints tokens to the contract). However, it does not have any access restrictions which leads to circumventing the vesting and further griefing attacks.

POC

Cirumventing vesting

The vesting can be circumvented by the user calling updateVestedTokens(user, amount) with a high amount which increases the amount storage variable, for example by 20.

The user can then claim all tokens already after the cliff period because the vested amount in calculateVestingClaim uses this modified amount value:

// amount has been inflated such that amount = oldAmount * 20
vestedAmount = amount.mul(5) / 100;

Breaking vestings for others

A malicious user can break other investors vestings by calling updateVestedTokens(victim, newAmount = type(uint256).max - amount) such that the new amount storage field is set close to the maximum possible integer value. When the victim tries to claimVestedTokens, it will either overflow and revert in calculateVestingClaim or the vested amount will be much larger than the balance of the vesting contract and revert there.

Impact

Vesting periods can be shortened to the cliff and vestings for other investors can be modified such that they are unable to claim anything. Both actions are economically useful as they either acquire more tokens for the attacker faster or prevent a high circulating supply (increasing the token price) by denying other investors to claim.

Recommendation

Add access restrictions to this function. It should only be callable from FSD.sol.

Handle

cmichel

Vulnerability details

The FSDVesting.claimVestedTokens function tokenizes the conviction only if the current claimed amount (tokenClaim) equals the total vested amount (amount).

// tokenClaim is vestedAmount - totalClaimed
uint256 tokenClaim = calculateVestingClaim();
if (amount == tokenClaim) {
    uint256 tokenId = fsd.tokenizeConviction(0); // @audit-info locked = 0
    fairSideConviction.transferFrom(address(this), msg.sender, tokenId);
}

This is only the case if no partial claims have been performed and the user claims once at the end of the vesting period.

Impact

I assume the idea to tokenize the conviction was that the vesting contract accrues conviction itself as the tokens vest inside of it.

It should always be possible to receive the tokenized conviction at the end of the vesting period, even if a user performed a partial claim before. Otherwise, users lose vested conviction.

Recommendation

Change the if (amount == tokenClaim) to if (amount <= totalClaimed) such that the accrued conviction is tokenized for all users at the end of their vesting schedule.

Handle

cmichel

Vulnerability details

In ERC20ConvictionScore._writeCheckpoint, when the checkpoint is overwritten (checkpoint.fromBlock == blockNumber), the new value is set to the memory checkpoint structure and never written to storage.

// @audit this is MEMORY, setting new convictionScore doesn't write to storage
Checkpoint memory checkpoint = checkpoints[user][nCheckpoints - 1];

if (nCheckpoints > 0 && checkpoint.fromBlock == blockNumber) {
    checkpoint.convictionScore = newCS;
}

Users that have their conviction score updated several times in the same block will only have their first score persisted.

POC

• User updates their conviction with updateConvictionScore(user)
• In the same block, the user now redeems an NFT conviction using acquireConviction(id). This calls _increaseConvictionScore(user, amount) which calls _writeCheckpoint(..., prevConvictionScore + amount). The updated checkpoint is not written to storage, and the user lost their conviction NFT. (The conviction/governance totals might still be updated though, leading to a discrepancy.)

Impact

Users that have their conviction score updated several times in the same block will only have their first score persisted.

This also applies to the total conviction scores TOTAL_CONVICTION_SCORE and TOTAL_GOVERNANCE_SCORE (see _updateConvictionTotals) which is a big issue as these are updated a lot of times each block.

It can also be used for inflating a user's conviction by first calling updateConvictionScore and then creating conviction tokens with tokenizeConviction. The _resetConviction will not actually reset the user's conviction.

Recommended Mitigation Steps

Define the checkpoint variable as a storage pointer:

Checkpoint storage checkpoint = checkpoints[user][nCheckpoints - 1];