search

code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

Wrong Type for Time Related Variable #157

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPair.sol#L69 https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPair.sol#L340

Vulnerability details

Impact

uint8, uint16 was used as Type to declare Time Related Variable in the LiquidationPair.sol contract, this totally wrong as except otherwise stated, seconds is the standard time frame in solidity and the number of digits needed will be too large for uint8 and uint16 to handle causing overflow errors and denial of service.

Proof of Concept

69.   uint16 _period;
...
340. _period = uint16(__period);

these two line of code in the LiquidationPair.sol contract shows a time related variable being declared and assigned with uint16 which is totally wrong

Tools Used

Solidity, manual review

Recommended Mitigation Steps

uint64 is still a better option as 2^64-1 would be large enough to handle time values

Assessed type

Math

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

raymondfam commented 1 year ago

__period isn't a Unix time. It's an index pointing to the current auction period.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Insufficient proof