016.md

Proud Rusty Mantis

Medium

Incorrect sign causes significantly less periods than intended in the long run

Vulnerability Detail

In VaultManager.updateBuyFeePID(), we have the following code:

        // when delta time is reached or PID is below last reference we reset reference
        else if (currentBlockts > nextCheckBlock) {
            //reset the increment max rate params
            buyPIDXhrAgo = buy_fee_PID;
            //set new block height +xhrs from now
            nextCheckBlock = currentBlockts + nextCheckBlockWindowDelta;
        }

Every nextCheckBlockWindowDelta, we can end up in this block to conduct the above state changes. However, the sign is incorrect as it checks for the current timestamp to be bigger than the next check timestamp.

Attack Path

1. We should be ending up in the above else if block every 4hrs to conduct the above state changes
2. A block length on Ethereum is 12 seconds (protocol will be deployed there)
3. Every 4 hrs, there will be a delay of 12 seconds (if we assume the function is called absolutely perfectly, which would be impossible)
4. Over a year, that will be a total delay of 432 minutes or over 7 hours, resulting in incorrect state

Impact

Incorrect state due to the delay

Mitigation

Include = in the check