Wrong penalty allocation computation

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L653 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L116 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L668

Vulnerability details

Impact

Lock quitting is one of the important functions of this protocol. Docs:

Lock quitting A non-expired lock may be quitted by the lock owner anytime. The lock cannot be delegated at the time of quitting and the quitter pays a penalty proportional to the remaining lock duration.

In some cases, it is possible to call quitLock() with zero penaltyAmount which is important for tokens with short decimals (for example: voting token with decimals = 0).

Proof of Concept

On constructor, the token can be with decimals < 18 :

  require(decimals <= 18, "Exceeds max decimals");

The penaltyRate defined by the following function:

function _calculatePenaltyRate(uint256 end)
        internal
        view
        returns (uint256)
    {
        return ((end - block.timestamp) * maxPenalty) / MAXTIME; 
// if  end - block.timestamp < 365
// (end - block.timestamp) * 10**18 / 365  < 10**18

// Example:
// 6 * 10**18 / 365  < 10**18

    }

which can return PenaltyRate < 10**18.

If the value will be under 60, the quitLock() can calculated the zero penaltyAmount which allowing an attacker to bypass the penalty mechanism which is important for tokens with short decimals.

 uint256 penaltyAmount = (value * penaltyRate) / 10**18;  
                                                                                                                 // Example if value = 60;
                                                                                                                // value * (10**18 * 6 / 365) / 10**18  < 1
                                                                                                                //  60 * 6/ 365 = 0

Tools Used

Manual analysis

Recommended Mitigation Steps

Add check for minimum value of token decimals in the constructor

[lacoop6tu]

The BPT which will be used has 18 decimals

[gititGoro]

Should be acknowledged as wardens don't know about BPT constraint.

[elnilz]

assuming that 106 is the minimal precision generally used across defi rounding would cause "loss" of 1 nominal unit of applicable fee if `penaltyRate<1012. In other words, remaining lock duration needs to be less than10*(-6)365*86400~32`. The user would thus be able to bypass quitLock penalty if she quits a lock with 32 seconds remaining duration or less which practically is irrelevant.

Designing a contract that works for all sorts of weird ERC20 implementations is not practical so imo this requires an inline comment and issue should be a QA instead of medium risk

[gititGoro]

Upon reconsideration, the 18 decimals is a red herring. In reality, since the penalty is expressed in 10^18, the division of 10^18 is just an order of magnitude neutralising operation required for fixed point arithmetic. The logic would apply whether the token chosen is USDC, WBTC or Dai.

Changing to invalid.

[gititGoro]

A subsequent post mitigation review was conducted that showed that for very small values (on the order of wei), rounding errors can lead to users avoiding quit penalties.

While this does mean the vector is potentially valid, it still remains invalid in practice because of gas fees. The issue here isn't about mainnet vs L2 but simply that gas fees exist at all.

For instance, if the token deposited is Eth and the fees avoided are about 10 wei per transaction then on Optimism, the gas price would be about 100,000 times the savings on fees.

Making a profit would either require having a consensus layer with absolute minimum gas fees or would require the deposited token to be hundreds of thousands, if not millions of times more valuable than Eth.

This is why the issue is valid on technical grounds only but of no practical relevance to the sponsor.

This sort of issue is akin to being out of scope since the constraints of the EVM can be assumed to always apply.