Owner can set very large lock duration

[code423n4]

Lines of code

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

Vulnerability details

Likelihood low, impact high.

The owner of JPEGLock.sol can intentionally or accidentally set lockTime to a very large lock duration. If a user's tokens are locked while this parameter is active, their tokens may be effectively permanently locked.

Additionally, since setLockTime does not emit an event, it will be difficult for off chain monitoring tools to detect if this parameter is incorrectly set.

Likelihood is mitigated since this function is permissioned, but incorrect time calculations are common (for example, accidentally using milliseconds rather than seconds), and the unintended impact of an error could be high.

Consider validating the maximum value of lockTime.

Test case to reproduce:

  it("owner can set very large lock duration", async () => {
    // The owner can intentionally or accidentally set a very large lock duration... 
    await jpegLock.setLockTime(days(365) * 10000);

    // If a user's tokens are locked while this very large lock duration is active, 
    // their tokens will be permanently locked.
    await jpeg.connect(alice).approve(jpegLock.address, units(500000000));
    await jpegLock.lockFor(alice.address, 0, units(250000000));

    // If the lock duration is sufficiently large, calls to `lockFor` will revert due to overflow:
    await jpegLock.setLockTime(ethers.constants.MaxUint256);
    await expect(jpegLock.lockFor(alice.address, 0, 250000000)).to.be.revertedWith("Arithmetic operation underflowed or overflowed");
  });

Example test cases for validation and event:

  it("owner cannot set lock duration above three years", async () => {
    await expect(jpegLock.setLockTime(days(365) * 3 +1)).to.be.revertedWith(
      "Invalid lock time"
    );
  });

  it("changing lock duration emits an event", async () => {
    // State changes to the `lockTime` variable do not emit an event. 
    const newDuration = days(365) * 2;
    await expect(jpegLock.setLockTime(newDuration)).to.emit(jpegLock, "SetLockTime").withArgs(newDuration);
  });

[spaghettieth]

Duplicate of #176

[dmvt]

Out of scope