ak1 - ProductManager.sol : function sync would revert when `available fund` is lesser than `refundAmount` amount.

[sherlock-admin]

ak1

medium

ProductManager.sol : function sync would revert when available fund is lesser than refundAmount amount.

Summary

function sync( calls the the function _complete

        if (program.versionComplete == 0 && programInfo.isComplete(currentOracleVersion.timestamp)) {
            versionComplete = _complete(self, product, programId);
        }

when we look at the _complete,

function _complete(
    ProductManager storage self,
    IProduct product,
    uint256 programId
) internal returns (uint256 versionComplete) {
    versionComplete = self.programs[programId].complete(product, self.programInfos[programId]);
    self.activePrograms.remove(programId);
}

this self.programs[programId].complete(, calls the following function.

 */
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);

    uint256 inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp);
    UFixed18 refundAmount = programInfo.amount.sum().muldiv(inactiveDuration, programInfo.duration);
    self.available = self.available.sub(refundAmount); ----------------------------------------------------------------------->> audit - will revert when any of market has less available fund
    address treasury = IIncentivizer(address(this)).treasury(programInfo.coordinatorId);
    self.settled[treasury] = self.settled[treasury].add(refundAmount);
}

Vulnerability Detail

Refer the summary section

Impact

sync would be DOSd when there are less available fund. This would be possible when we look at the claim logic where the prorata based value is used when there are less collateral value.

Code Snippet

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

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

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

revert at this line due to overflow https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L71

Tool used

Manual Review

Recommendation

Update the logic such that when there are less available fund, either continue or use that amount and continue for syncing.

[aktech297]

Escalate for 10 USDC.

Comment from lead judge why this was excluded.

Submission is insufficient. Does not specify the full problem and doesn't show when and how will this exactly occur if at all.

There is enough explanation about the sync. The issue explains the reverting of sync when there is insufficient funds with product.

This would happen when the the library function ProductManager.sync is called from the contract Incentivizer.sync.

during the sync process, ProductManager would tries to grab the product data of previous version of oracle with current product and looks for complete.

The complete would be called in order to stop the rewards from accruing. But, in the event of insufficient funds, the complete would revert as a result the whole sync would revert.

as explained in the issue details with relevant code flow, revert would happen as mentioned in Code Snippet section.

as mentioned from See issue 002 of Veridise report. By design. sending pro rata when funds are not enough, there are possibilities that product may not have sufficient funds with it. if that is the case, that product can never be synced due to reverting.

This can be High issue too.

[sherlock-admin]

Escalate for 10 USDC.

Comment from lead judge why this was excluded.

Submission is insufficient. Does not specify the full problem and doesn't show when and how will this exactly occur if at all.

There is enough explanation about the sync. The issue explains the reverting of sync when there is insufficient funds with product.

This would happen when the the library function ProductManager.sync is called from the contract Incentivizer.sync.

during the sync process, ProductManager would tries to grab the product data of previous version of oracle with current product and looks for complete.

The complete would be called in order to stop the rewards from accruing. But, in the event of insufficient funds, the complete would revert as a result the whole sync would revert.

as explained in the issue details with relevant code flow, revert would happen as mentioned in Code Snippet section.

as mentioned from See issue 002 of Veridise report. By design. sending pro rata when funds are not enough, there are possibilities that product may not have sufficient funds with it. if that is the case, that product can never be synced due to reverting.

This can be High issue too.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[KenzoAgada]

Issue is invalid.

It claims the problem is in the line:

self.available = self.available.sub(refundAmount);

But I see no option of it underflowing. One can see where self.available is updated - only when initializing the program, settling unclaimed, and finally when it finishes.

The escalation says that it can underflow like mentioned in VAR002 of Veridise audit. But that issue talked about not enough funds in BalancedVault assets, while here we're talking about Program rewards. Two totally different areas and that issue is irrelevant to this submission.

[aktech297]

The escalation did not mention that the same reason can happen to this issue case also. But it refers one of the situation where insufficient of funds are expected.

when we see the self.available, it has been updated in following places.

During initilizing initialize

function initialize(Program storage self, ProgramInfo memory programInfo) internal {
    self.available = programInfo.amount.sum();
}

And it is deducted in following places.

during settle and complete

here, complete would be last one that can happen during sync. But the settle can be called by each account for syncAccount which assumes that the sync already been called. But, this will not be the case always. syncAccount would be triggered by settleAccount in Product.

The call flow would be, settleAccount -> _settleAccount .

Note, settleAccount is external and can be called by anyone with valid account.

when account is synced, each time the self.available would be deducted.

now, lets come to the complete call , the flow already given in above discussion. lets see only the function 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);

    uint256 inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp);
    UFixed18 refundAmount = programInfo.amount.sum().muldiv(inactiveDuration, programInfo.duration);
    self.available = self.available.sub(refundAmount);
    address treasury = IIncentivizer(address(this)).treasury(programInfo.coordinatorId);

This function, look for refund amount by taking the inactive duration multiples by the programInfo.amount.sum.

if the inactive duration is more then the refund amount also would be more.

when we see the self.available, it is already deducted during the syncAccount call. but when we see the programInfo.amount.sum and self.available, both are independent each other, the code tries to subtract the refundAmount from the self.available, as I mentioned, that for long duration the refund amount value would be more, but the self.avaulable is already deduced when syncAccount call.

above situation would lead to refundAmount > self.available . In this case, the self.available = self.available.sub(refundAmount); will revert.

[aktech297]

I don't know how this will resolve .

[KenzoAgada]

I've read the comment, I don't think the issue or escalation show why an underflow is a real possibility. The latest comment says we see the programInfo.amount.sum and self.available, both are independent each other. But when a program is initialized it sets self.available = programInfo.amount.sum() and then the available amount is updated using _unsettled. I think issue should remain invalid.

[aktech297]

may be this one is missed from attention:

the code tries to subtract the refundAmount from the self.available, as I mentioned, that for long duration the refund amount value would be more, but the self.avaulable is already deduced when syncAccount call.

above situation would lead to refundAmount > self.available . In this case, the self.available = self.available.sub(refundAmount); will revert.

[KenzoAgada]

It has not missed from attention, just seems false. From the above explanation it can be understood that the function will always revert since "self.available is already deduced when syncAccount call".

(I'm not saying I agree with this.)

If so, have you checked why are the tests not failing? Can you code a POC that shows this?

[aktech297]

I duly respect judge decision on this.

but, I don't think codes POC is necessary given the nature of the issue which is pretty straightforward.

Most of the issues are judged not only with code POC but with detailed explanation about the code flow.

I can show a codes POC. but not immediately. As I have loaded with couple more projects, I can work on it in a week or two and share it.

Thanks!

[jacksanford1]

I have doubts that this issue would be valid even if the POC is correct. @aktech297

Impact only explains a temporary DOS which is not valid in Sherlock's judging rules: https://docs.sherlock.xyz/audits/judging/judging

[aktech297]

But, one case could be, there is some malicious transaction is happening which could be material loss, the contract have to be rebalanced in order to stop this transaction. but, due to this issue, contract is not able to be rebalanced. The team has to sit and analyze and the issues and then arrive for fix by finding this, this could take a while. Till then whatever the malicious transaction happens, could impact on the protocol.

[securitygrid]

@jacksanford1 if this issue would be valid, it's impact is critical. you can see Impact section in #144. This is not a temporary DOS. @aktech297 you should provide a poc to prove it. if it's valid, a solo high.

[aktech297]

@securitygrid thanks for referring the issue.

I believe there are sufficient explanation from my side with relevant code blocks.I am aware that issues can be submitted with or without POC. But, I am happy to share the POC.

Since I am am occupied with few more projects , not able to commit on this immediately, it could take a week or two weeks.

I understand the complexity of this project. If it can not be arrived a solution from judging side immediately, I would suggest to have another eye on this issue either from sponser.

Thanks!

[jacksanford1]

Ok, we can have @arjun-io read through the issue description again and try to see if this potential Critical issue is realistic.

But the initial response from the judge is that the issue description is insufficient, and @aktech297 these escalations need to be resolved by Friday, so you need to get the POC in soon, otherwise the issue will likely resolve as invalid because no one else could figure out how it works (assuming Arjun can't see the scenario either).

[arjun-io]

Is this similar to Veridise 4.1.6? We'd need a clear PoC where this calculation can actually underflow. This would point to a larger accounting issue rather than just protecting this with a simple check.

[jacksanford1]

Ok, then a POC is needed @aktech297. Need it by Friday. If you are correct, this is likely a High Solo and I'd expect at least $5k-$10k for it, so it may be worth your time to create the POC.

[jacksanford1]

Result: Invalid Unique No POC was provided for this issue so unfortunately there is not enough evidence to upgrade the issue.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• aktech297: rejected