rvierdiiev - Incentivizer.complete doesn't sync last oracle version

[sherlock-admin]

rvierdiiev

medium

Incentivizer.complete doesn't sync last oracle version

Summary

Incentivizer.complete doesn't sync last oracle version, because of that product.currentVersion returns outdated result and program owner can receive more funds back then he should.

Vulnerability Detail

Incentivizer.complete function is allowed to be called by product coordinator in order to finish reward program. As result part of funds will be returned to the coordinator and another part should stay to pay rewards. This function calls program manager to finish program, which will then call program.

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

    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);
        self.settled[treasury] = self.settled[treasury].add(refundAmount);
    }

In order to understand how many time program is already active, product.latestVersion() is used. The problem is that this version can be stale and it needs to be updated to the current lates oracle config using sync function. As result product.latestVersion() function will show old value and coordinator will receive more funds back.

Impact

Coordinator will receive more funds back and less rewards will be distributed.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

You need to sync product's oracle to get last oracle version.

[KenzoAgada]

Doesn't seem like a medium issue. Program manager is anyway allowed to complete the program early whenever he wishes; is there justification in saying he's "receiving more funds than he should"? He simply completes the program as per it's last oracle update. Considering as a valid informational.

[arjun-io]

The issue here is that if we use the current version it leads to accounting issues because part of the time period will be after the program creator's desired duration. Also as Kenzo said, since he program manager can close the program early at any time this is not really an issue.

[rvierdiiev]

Escalate for 10 usdc. I would say it in another way then.

In case if he uses not up to date version then he cuts already earned rewards for users. I have described that in impact section. In case if program exists and it claims to pay funds and on timestamp 100 manager decided to close program, then it should not be closed on timestamp 90, right?

product.latestVersion() updates when some actions are done. so it's possible that for some time no actions will be performed and as result less amount of rewards will be paid for users, even that manager doesn't mean that. he just wanted to finish his program on timestamp 100 and distribute all rewards that he claimed to. But actually program has finished on timestamp 90 and user received less rewards.

This is loss of yields, which is usually medium severity.

[sherlock-admin]

Escalate for 10 usdc. I would say it in another way then.

In case if he uses not up to date version then he cuts already earned rewards for users. I have described that in impact section. In case if program exists and it claims to pay funds and on timestamp 100 manager decided to close program, then it should not be closed on timestamp 90, right?

product.latestVersion() updates when some actions are done. so it's possible that for some time no actions will be performed and as result less amount of rewards will be paid for users, even that manager doesn't mean that. he just wanted to finish his program on timestamp 100 and distribute all rewards that he claimed to. But actually program has finished on timestamp 90 and user received less rewards.

This is loss of yields, which is usually medium severity.

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]

I think the original issue can be deemed invalid due to the misunderstanding of how the Incentivizer works. The issue says in the title and Summary:

Incentivizer.complete doesn't sync last oracle version, because of that product.currentVersion returns outdated result and program owner can receive more funds back then he should.

But as the sponsor explained in a few places including here, syncing to the current oracle version will make the program last longer than the intended duration. So as it is, I think the issue is invalid.

Regarding the escalation, it is "making" the issue (putting the emphasis) more like issue #226 and it's escalation. The comments I wrote there (please read) pretty much apply here. So for the same reasoning I would maintain invalidity. And additionally, the issue might be deemed invalid simply because it originally misidentified the problem.

[jacksanford1]

@rvierdiyev Any response to Kenzo's latest comment?

[rvierdiiev]

hello @jacksanford1 i agree that this issue is about same as described in #226 and also watson correctly provided info about situation when this issue can have big impact here: https://github.com/sherlock-audit/2023-05-perennial-judging/issues/226#issuecomment-1627859014

when sherlock judges loss of yields for any user, then it's usually medium severity in this case, not only one 1 user losses yields, but all participants of a program so i also think that this is valid issue.

[jacksanford1]

Hmm I think the original issue description seems quite different than #226 and I don't like that there is some potential posturing to make it more similar.

Is the coordinator considered to be a trusted role? If the coordinator has the power to end the program early anyways, I don't see how it could be a serious loss of funds if that occurs accidentally and the rewards end up back with the coordinator (who could manually distribute them properly if this bugged situation did occur).

Will let @KenzoAgada provide thoughts.

[KenzoAgada]

• The original submission was pretty much wrong ("Incentivizer.complete doesn't sync last oracle version" - it shouldn't, see sponsor comment)
• The escalation kinda "changed" it and presented a more general issue, same as 226 - the issues are similar but this is somewhat unfair as 226 is different to how here the original submission and issue were presented.
• Indeed (only) the coordinator has permission to end program early, and funds go back to the treasury address set for this coordinator

Due to these I think issue is invalid.

[jacksanford1]

Agreed. I don't see a case where a large amount of funds ends up with a malicious actor. But open to ideas @rvierdiyev

[jacksanford1]

Result: Invalid Unique Reasoning can be found in Kenzo's last comment.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• rvierdiyev: rejected