sherlock-audit / 2023-12-dodo-gsp-judging

6 stars 5 forks source link

bareli - wrong implementation of timeElapsed. #146

Closed sherlock-admin closed 10 months ago

sherlock-admin commented 10 months ago

bareli

medium

wrong implementation of timeElapsed.

Summary

Here we are using timeElapsed as uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMPLAST; As we are using a modules to 232 so _BLOCK_TIMESTAMPLAST will be in range between 0 and 232 . so After 2**32 we will have the same value.

Vulnerability Detail

uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_; 

Impact

if (timeElapsed > 0 && _BASERESERVE != 0 && _QUOTERESERVE != 0) { _BASE_PRICE_CUMULATIVELAST += getMidPrice() * timeElapsed; } so timeElapsed cannot be zero and_BASE_PRICE_CUMULATIVELAST cannot be implemented.

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L86 function _twapUpdate() internal { // blockTimestamp is the timestamp of the current block uint32 blockTimestamp = uint32(block.timestamp % 2*32); // timeElapsed is the time elapsed since the last update uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMPLAST; // if timeElapsed is greater than 0 and the reserves are not 0, update the twap price if (timeElapsed > 0 && _BASERESERVE != 0 && _QUOTERESERVE != 0) { _BASE_PRICE_CUMULATIVELAST += getMidPrice() timeElapsed; } // update the last block timestamp _BLOCK_TIMESTAMPLAST = blockTimestamp; }

Tool used

Manual Review

Recommendation

nevillehuang commented 10 months ago

Invalid, _BLOCK_TIMESTAMP_LAST_ is the previous cached twap update timestamp, so no issue here, logic is correct as seen here