All position transfers will fail because of a flawed formula in the PointsModule

Summary

The idea of the protocol is to have transferable positions, but due to the wrong formula used in the PointsModule, unlock time will be set to a very big number and prevent the transfers.

Vulnerability Detail

All users in the system are minted additional points when performing the operations in the protocol:

deposit LP
withdraw LP
open long position

They are initially locked and cannot be transferred or burned. Since these points are connected to a user this restriction is applied to the whole position and collateral tokens associated with the position won’t be transferrable for the period when they are locked.

The formula used to extend the lock time can be found in PointsModule::_setMintUnlockTime:

/// @notice Sets the unlock time for newly minted points.
/// @dev    If the user has existing locked points, then the new unlock time is calculated based on the existing locked points.
///         The newly minted points are included in the `lockedAmount` calculation.
function _setMintUnlockTime(address account, uint256 mintAmount) internal returns (uint256 newUnlockTime) {
    uint256 lockedAmount = _lockedAmount[account];
    uint256 unlockTimeBefore = unlockTime[account];

    if (unlockTimeBefore <= block.timestamp) {
        newUnlockTime = block.timestamp + unlockTaxVest;
    } else {
        uint256 newUnlockTimeAmount = (block.timestamp + unlockTaxVest) * mintAmount;
        uint256 oldUnlockTimeAmount = unlockTimeBefore * (lockedAmount - mintAmount);
        newUnlockTime = (newUnlockTimeAmount + oldUnlockTimeAmount) / lockedAmount;
    }

    unlockTime[account] = newUnlockTime;
}

Important to note that the unlockTaxVest will be set to 365 days from the deployment script.

We can see that the tokens will be locked for at least 365 days and not be eligible to be withdrawn.

On every new mint the flow will enter the else statement and additionally increase the lock time even more by multiplying it to mintAmount.

Impact

It is crucial for these types of protocols to not prevent users from selling their positions, because it will save them from bad consequences and will have more positions closed before being eligible for liquidation.

Code Snippet

PointsModule.sol

/// @notice Sets the unlock time for newly minted points.
/// @dev    If the user has existing locked points, then the new unlock time is calculated based on the existing locked points.
///         The newly minted points are included in the `lockedAmount` calculation.
function _setMintUnlockTime(address account, uint256 mintAmount) internal returns (uint256 newUnlockTime) {
    uint256 lockedAmount = _lockedAmount[account];
    uint256 unlockTimeBefore = unlockTime[account];

    if (unlockTimeBefore <= block.timestamp) {
        newUnlockTime = block.timestamp + unlockTaxVest;
    } else {
        uint256 newUnlockTimeAmount = (block.timestamp + unlockTaxVest) * mintAmount;
        uint256 oldUnlockTimeAmount = unlockTimeBefore * (lockedAmount - mintAmount);
        newUnlockTime = (newUnlockTimeAmount + oldUnlockTimeAmount) / lockedAmount;
    }

    unlockTime[account] = newUnlockTime;
}

Tool used

Manual Review

Recommendation

Consider fixing the formula to lock only when the first mint from account occurs or completely remove the PointsModule since the developers confirmed that there is no defined usage of the points.

Duplicate of #200

sherlock-audit / 2023-12-flatmoney-judging

SBSecurity - All position transfers will fail because of a flawed formula in the PointsModule #269