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

12 stars 7 forks source link

_getNextObservationIndex() Random use of timestamp to determine the currentTime can be manipulated bacause of dangerous strict equalities #450

Closed code423n4 closed 12 months ago

code423n4 commented 12 months ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L367 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L381 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L529

Vulnerability details

Impact

The use of strict equalities can be easily manipulated by an attacker. Miners may attempt to manipulate the timestamp.

Proof of Concept

File: TwabLib.sol Code Link: https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L367 Code: if (newestObservation.timestamp == currentTime) {

File: TwabLib.sol Code Link: https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L381 Code: if (currentPeriod == 0 || currentPeriod > newestObservationPeriod) {

File: TwabLib.sol Code Link: https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L529 Code: if (afterOrAtObservation.timestamp == _targetTime) {

Tools Used

Manual Review

Recommended Mitigation Steps

Don't use strict equality to determine the timestamp currenttime. instead, use Multiple Time Sources not relying solely on block.timestamp, consider using multiple time sources or external oracles to obtain a more reliable and tamper-resistant timestamp. Try to Implement time window validation to account for slight variations in the timestamp. Instead of strict equality checks, consider using ranges or thresholds to accommodate small discrepancies in the timestamps. Also, leverage external consensus mechanisms, such as timestamping services or decentralized timestamping protocols, to obtain timestamps that are more resistant to manipulation by individual miners. Finally, make the contract's behavior dependent on the gas limit instead of the timestamp. Since miners cannot directly manipulate the gas limit, this can provide a more reliable measure for time-dependent operations.

Assessed type

Timing

c4-judge commented 12 months ago

Picodes marked the issue as unsatisfactory: Insufficient quality