Users should not use `quitLock ()` function when `unlock()` is set.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L632-L659 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L161-L164

Vulnerability details

Impact

In this case, the quitLock () function can be abused as it would allow users to exit the locking mechanism at anytime. As such, the entire objective of a locking mechanism would not be realized.

Proof of Concept

The quitLock () function is used to quit locking at anytime and the quitter has to pay a penalty as shown below in the code snippet https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L632-L659 However, the contract provides the ability to removequitLock by setting the penalty to 0 as shown in this code https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L161-L164 When the penalty is set to 0 users should not be allowed to use the the quitLock () function as the quitLock () would allow users to exit the locking mechanism at anytime. As such, the entire objective of a locking mechanism would not be realized.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider in the quitLock () to put a require statement as: require (maxPenalty > 0) to only allow usage of this function when the maxPenalty is still active.

[lacoop6tu]

This is an expected behaviour, unlock() is only used if we need to migrate and thus allowing users to immediately withdraw without penalty

[gititGoro]

This seems very intentional. Marking invalid.