code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L29-L31 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L142-L145 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L435-L446

Vulnerability details

Impact

The PERIOD_OFFSET has many important use cases and since it was stated in the comment that it has to be in the past, then it must never be in the future, as it will affect the output of many function it is being passed as a parameter into.

Proof of Concept

No check to ensure that PERIOD_OFFSET can only be in the past, so there lies a possibility that the deployer could set it into the future.

  constructor(uint32 _periodLength, uint32 _periodOffset) {
    PERIOD_LENGTH = _periodLength;
    PERIOD_OFFSET = _periodOffset;
  }

Tools Used

Manual review.

Recommended Mitigation Steps

Subtract some time from the current timestamp to give the PERIOD_OFFSET, this gives a better assurance that it will always be in the past.

  constructor(uint32 _periodLength, uint32 _periodOffsetSeconds) {
    PERIOD_LENGTH = _periodLength;
    PERIOD_OFFSET = block.timestamp - _periodOffsetSeconds;
  }

Assessed type

Timing

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 11 months ago

Picodes marked the issue as grade-a