[WP-H13] `LockPosition` can be overwritten by new `lockFor()` which leads to user's funds loss

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L49-L63

Vulnerability details

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L49-L63

function lockFor(
    address _account,
    uint256 _nftIndex,
    uint256 _lockAmount
) external onlyOwner nonReentrant {
    jpeg.safeTransferFrom(_account, address(this), _lockAmount);

    positions[_nftIndex] = LockPosition({
        owner: _account,
        unlockAt: block.timestamp + lockTime,
        lockAmount: _lockAmount
    });

    emit Lock(_account, _nftIndex, _lockAmount);
}

Based on the context, and given the volatility of the NFT market, the DAO may give the same NFT at different prices at different times.

When that happens, in the current implementation, the locked JPEG tokens belonging to the previous owner will be frozen in the contract as the record will be overwritten by the new lock.

PoC

Given:

• creditLimitRate: 50%
• valueIncreaseLockRate: 30%

1. DAO setPendingNFTValueETH() for Alice's NFT#1 with a price of 10 ETH;
2. Alice finalizePendingNFTValueETH() and locked 1.5 ETH worth of JPEG tokens;
3. DAO adjusted the assessment and setPendingNFTValueETH() for Alice's NFT#1 with a price of 20 ETH;
4. Alice finalizePendingNFTValueETH() and locked 3 ETH worth of JPEG tokens.

The 1.5 ETH worth of JPEG tokens locked in step 2 won't be able to be unlocked as the LockPosition was overwritten in step 4.

Recommendation

Change to:

function lockFor(
    address _account,
    uint256 _nftIndex,
    uint256 _lockAmount
) external onlyOwner nonReentrant {
    LockPosition memory position = positions[_nftIndex];
    if (position.lockAmount > 0) {
        jpeg.safeTransfer(position.owner, position.lockAmount);
    }
    jpeg.safeTransferFrom(_account, address(this), _lockAmount);

    positions[_nftIndex] = LockPosition({
        owner: _account,
        unlockAt: block.timestamp + lockTime,
        lockAmount: _lockAmount
    });

    emit Lock(_account, _nftIndex, _lockAmount);
}

[spaghettieth]

Duplicate of #10