nobody2018 - Program.complete may cause subtraction overflow in some cases

[sherlock-admin]

nobody2018

high

Program.complete may cause subtraction overflow in some cases

Summary

Program.complete is called when an incentive program is completed. There is [a line of code](https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L69) inside the function to calculate the inactiveDuration, where it is possible to overflow/underflow in some cases.

Vulnerability Detail

Let's see the code of Program.complete:

function complete(
        Program storage self,
        IProduct product,
        ProgramInfo memory programInfo
    ) internal returns (uint256 versionComplete) {
        uint256 versionStarted = self.versionStarted;
        versionComplete = Math.max(versionStarted, product.latestVersion());
        self.versionComplete = versionComplete;

        IOracleProvider.OracleVersion memory fromOracleVersion = product.atVersion(versionStarted);
        IOracleProvider.OracleVersion memory toOracleVersion = product.atVersion(versionComplete);
        //@audit revert due to (toOracleVersion.timestamp - fromOracleVersion.timestamp) > programInfo.duration
->      uint256 inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp);
        ...
    }

If programInfo.duration is less than (toOracleVersion.timestamp - fromOracleVersion.timestamp), then tx will revert. The flow of the entire tx is as follows:

Product._settle()                           //L84  in Product.sol
  _controller.incentivizer().sync(currentOracleVersion)     //L102 in Product.sol
    _products[product].sync(product, currentOracleVersion)  //L100 in Incentivizer.sol
      _complete(self, product, programId)           //L91  in ProductManager.sol
        self.programs[programId].complete(product, self.programInfos[programId])    //L200  in ProductManager.sol
          Program.complete                  //L57  in Program.sol

Here are two use cases to prove that programInfo.duration will be less than (toOracleVersion.timestamp - fromOracleVersion.timestamp).

The first use case: programInfo.duration is assigned when the program is created, and the duration cannot be guaranteed to be a multiple of (toOracleVersion.timestamp - fromOracleVersion.timestamp). For simplicity, assume that the timestamp of the adjacent OraclefVersion is 60s, and the programInfo.duration is 150s.

OraclefVersion0.timestamp = 0       //program is started in OraclefVersion0
OraclefVersion1.timestamp = 60
OraclefVersion2.timestamp = 120
OraclefVersion3.timestamp = 180

In this case programInfo.duration is less than (toOracleVersion.timestamp - fromOracleVersion.timestamp). Because 150 < (180 - 0).

The second use case: The product is paused or the external oracle is not working properly (sequencer is down on L2). This will cause all  OracleVersions to be skipped during this period.

//L84 in Product.sol
    function _settle() private returns (IOracleProvider.OracleVersion memory currentOracleVersion) {
        ...
        // Get current oracle version
->      currentOracleVersion = _sync(); //get newest OracleVersion from chainlink
        ...
        // Get settle oracle version
        uint256 _settleVersion = _position.pre.settleVersion(currentOracleVersion.version);
        IOracleProvider.OracleVersion memory settleOracleVersion = _settleVersion == currentOracleVersion.version
            ? currentOracleVersion // if b == c, don't re-call provider for oracle version
            : atVersion(_settleVersion);
        // Initiate
->      _controller.incentivizer().sync(currentOracleVersion);  //newest OracleVersion
    ...
    }

For simplicity, assume that the timestamp of the adjacent OraclefVersion is 60s, and the programInfo.duration is 600s.

OraclefVersion0.timestamp = 0       //program is started in OraclefVersion0
OraclefVersion1.timestamp = 60
OraclefVersion2.timestamp = 120
OraclefVersion3.timestamp = 180     //chainlink don't work properly or product pasued.
not work                    240
...
OraclefVersionCurrent.timestamp = 780   //Back to normal

In this case programInfo.duration is less than (toOracleVersion.timestamp - fromOracleVersion.timestamp). Because 600 < (780 - 0).

Impact

Although both use cases are uncommon, if one of them occurs, the product would be useless. No one can open/close positions/liquidate, nor withdraw collateral. The impact is huge. Because Product._settle() is the core function. Most interfaces that users can call depend on it, such as openTakeFor/closeTakeFor/openMakeFor/closeMakeFor/closeAll in Product.sol, withdrawFrom/liquidate in Collateral.sol.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L69

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L96

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L589

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L69

Tool used

Manual Review

Recommendation

IOracleProvider.OracleVersion memory fromOracleVersion = product.atVersion(versionStarted);
        IOracleProvider.OracleVersion memory toOracleVersion = product.atVersion(versionComplete);

-       uint256 inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp);
+       uint256 inactiveDuration = 0;
+   if (programInfo.duration > (toOracleVersion.timestamp - fromOracleVersion.timestamp)) {
+       inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp);
+   }
        UFixed18 refundAmount = programInfo.amount.sum().muldiv(inactiveDuration, programInfo.duration);
        self.available = self.available.sub(refundAmount);

[KenzoAgada]

While the general issue was already reported in the Veridise audit (006), where the protocol responded that it can't underflow, this submission correctly identifies that it can underflow in the "second use case" mentioned above; if product is paused or the external oracle is not working properly (sequencer is down on L2).

[arjun-io]

The second use case is similarly not possible because of what is described in the Veridise audit. The end version (toOracleVersion) is determined by the following snippet:

versionComplete = Math.max(versionStarted, product.latestVersion());

In this case, product.latestVersion() will be OraclefVersion3 not OraclefVersionCurrent. This is because sync() is called before the product.latestVersion is transitioned.