FairSide contest - cmichel's results

Handle

cmichel

Vulnerability details

Vulnerability Details

In tokenizeConviction when locked > 0 the amount is first transferred from the user using an internal call to _transfer(msg.sender, address(fairSideConviction), locked);. It is then transferred a second time from the user in the fairSideConviction.createConvictionNFT call:

function createConvictionNFT(
    address user,
    uint256 score,
    uint256 locked,
    bool isGovernance
) external override returns (uint256) {
    if (locked > 0) {
        cs.locked = locked;
        // steals a second time
        FSD.safeTransferFrom(user, address(this), locked);
    }
}

Impact

The locked balance is transferred twice from the user instead of once, stealing their balance.

Recommended Mitigation Steps

Remove the transfer in createConvictionNFT.

Handle

cmichel

Vulnerability details

Vulnerability Details

The TOTAL_GOVERNANCE_SCORE is supposed to track the sum of the credit scores of all governors.

In ERC20ConvictionScore._updateConvictionScore, when the user does not fulfill the governance criteria anymore and is therefore removed, the governanceDelta should be negative but it's positive.

isGovernance[user] = false;
governanceDelta = getPriorConvictionScore(
    user,
    block.number - 1
);

It then gets added to the new total:

uint224 totalGCSNew =
    add224(
        totalGCSOld,
        governanceDelta,
        "ERC20ConvictionScore::_updateConvictionTotals: conviction score amount overflows"
    );

Impact

The TOTAL_GOVERNANCE_SCORE tracks wrong data leading to issues throughout all contracts like wrong FairSideDAO.totalVotes data which can then be used for anyone to pass proposals in the worst case. Or totalVotes can be arbitrarily inflated and break the voting mechanism as no proposals can reach the quorum (percentage of totalVotes) anymore.

Recommended Mitigation Steps

Return a negative, signed integer for this case and add it to the new total.

Handle

cmichel

Vulnerability details

Vulnerability Details

The name pendingWithdrawals indicates that this storage variable tracks the withdrawals that need yet to be paid out which also matches the behavior in _increaseWithdrawal. So it should be decreased when withdrawing in withdraw but it is not.

Impact

The getReserveBalance consistently under-reports the actual reserve balance which leads to wrong mint amounts being used in the FSD.mint calculation.

Recommendation

Decrease pendingWithdrawals by the withdrawn amount.

Handle

cmichel

Vulnerability details

Vulnerability Details

When _reserveDelta is negative in ABC._calculateDeltaOfFSD the following branch is executed:

if (_reserveDelta < 0) {
    uint256 capitalPostWithdrawal =
        capitalPool.sub(uint256(_reserveDelta));

The type cast to uint256 is purely a reinterpretation of the underlying bytes, it does not compute the absolute value. Which means uint256(_reserveDelta) will be a huge value (2^256 - abs(_reserveDelta)) due to two's complement encoding. This is always greater than the capitalPool and the transaction will revert because of the underflow in SafeMath.sub.

Impact

The FSD.burn function always calls it with a negative value which would break the burn function among other functions. (Why is this not caught in a test though?)

Recommendation

Multiply _reserveDelta by -1 to get the absolute value and then the type-cast to uint256 will be safe.

Handle

cmichel

Vulnerability details

Vulnerability Details

In ERC20ConvictionScore._updateConvictionScore, when the user does not fulfill the governance criteria anymore, the governanceDelta is the old conviction score of the previous block.

isGovernance[user] = false;
governanceDelta = getPriorConvictionScore(
    user,
    block.number - 1
);

The user could increase their conviction / governance score first in the same block and then lose their status in a second transaction, and the total governance conviction score would only be reduced by the previous score.

Example: Block n - 10000: User is a governor and has a credit score of 1000 which was also contributed to the TOTAL_GOVERNANCE_SCORE Block n:

• User updates their own conviction score using public updateConvictionScore function which increases the credit score by 5000 based on the accumulated time. The total governance credit score increased by 5000, making the user contribute 6000 credit score to governance in total.
• User transfers their whole balance away, the balance drops below governanceMinimumBalance and user is not a governor anymore. The governanceDelta update of the transfer should be 6000 (user's whole credit score) but it's only 1000 because it takes the snapshot of block n - 1.

Impact

The TOTAL_GOVERNANCE_SCORE score can be inflated this way and break the voting mechanism in the worst case as no proposals can reach the quorum (percentage of totalVotes) anymore.

Recommended Mitigation Steps

Use the current conviction store which should be governanceDelta = checkpoints[user][userCheckpointsLength - 1].convictionScore

Handle

cmichel

Vulnerability details

Vulnerability Details

In tokenizeConviction, when locked == 0 the _updateConvictionScore(msg.sender, 0) function is called to update the user's conviction, however the delta is not added to the total credit / governance score.

Impact

The TOTAL_CONVICTION_SCORE and TOTAL_GOVERNANCE_SCORE track wrong data leading to issues throughout all contracts, especially with on-chain governance voting.

Recommended Mitigation Steps

Call updateConvictionScore instead of _updateConvictionScore.

Handle

cmichel

Vulnerability details

Vulnerability Details

The credit score of the special address(type(uint160).max) is supposed to represent the sum of the credit scores of all users that are governors. But any user can directly transfer to this address increasing its balance and accumulating a credit score in _updateConvictionScore(to=address(uint160.max), amount). It'll first write a snapshot of this address' balance which should be very low:

// in _updateConvictionScore
_writeCheckpoint(user, userNum, userNew) = _writeCheckpoint(TOTAL_GOVERNANCE_SCORE, userNum, checkpoints[user][userNum - 1].convictionScore + convictionDelta);

This address then accumulates a score based on its balance which can be updated using updateConvictionScore(uint160.max) and breaks the invariant.

Impact

Increasing it might be useful for non-governors that don't pass the voting threshold and want to grief the proposal voting system by increasing the quorumVotes threshold required for proposals to pass. (by manipulating FairSideDAO.totalVotes). totalVotes can be arbitrarily inflated and break the voting mechanism as no proposals can reach the quorum (percentage of totalVotes) anymore.

Recommended Mitigation Steps

Disallow transfers from/to this address. Or better, track the total governance credit score in a separate variable, not in an address.

Handle

cmichel

Vulnerability details

Vulnerability Details

There is no check in FSDNetwork.getEtherPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the membership and cost share pricing.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = ETH_CHAINLINK.latestRoundData();
require(
    timeStamp != 0,
    "ChainlinkOracle::getLatestAnswer: round is not complete"
);
require(
    answeredInRound >= roundID,
    "ChainlinkOracle::getLatestAnswer: stale data"
);
require(price != 0, "FSDNetwork::getEtherPrice: Chainlink Malfunction");

Handle

cmichel

Vulnerability details

Vulnerability Details

There are two issues with the governance checks when acquiring them from an NFT:

Missing balance check

The governance checks in _updateConvictionScore are:

!isGovernance[user]
&& userConvictionScore >= governanceThreshold 
&& balanceOf(user) >= governanceMinimumBalance;

Whereas in acquireConviction, only userConvictionScore >= governanceThreshold is checked but not && balanceOf(user) >= governanceMinimumBalance.

else if (
    !isGovernance[msg.sender] && userNew >= governanceThreshold
) {
    isGovernance[msg.sender] = true;
}

the wasGovernance might be outdated

The second issue is that at the time of NFT creation, the governanceThreshold or governanceMinimumBalance was different and would not qualify for a governor now. The NFT's governance state is blindly appplied to the new user:

if (wasGovernance && !isGovernance[msg.sender]) {
    isGovernance[msg.sender] = true;
}

This allows a user to circumvent any governance parameter changes by front-running the change with an NFT creation.

Impact

It's easy to circumvent the balance check to become a governor by minting and redeeming your own NFT. One can also circumvent any governance parameter increases by front-running these actions with an NFT creation and backrunning with a redemption.

Recommended Mitigation Steps

Add the missing balance check in acquireConviction. Remove the wasGovernance governance transfer from the NFT and solely recompute it based on the current governanceThreshold / governanceMinimumBalance settings.