search

sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

ast3ros - Incentive can be miscalculated #226

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

ast3ros

medium

Incentive can be miscalculated

Summary

The versionStart and versionComplete of an incentive program can be mismatched with the intended start and end time if the product is not settled frequently.

Vulnerability Detail

The actual duration of an incentive program is determined by versionStart and versionComplete in the program struct.

    struct Program {
        /// @dev Mapping of latest rewards settled for each account
        mapping(address => UFixed18) settled;

        /// @dev Total amount of rewards yet to be claimed
        UFixed18 available;

        /// @dev Oracle version that the program started, 0 when hasn't started
        uint256 versionStarted;

        /// @dev Oracle version that the program completed, 0 is still ongoing
        uint256 versionComplete;
    }

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

On the other hand, the intended start time of the program can be assigned by the product manager when creating the program in the ProgramInfo struct.

    uint256 start;

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/interfaces/types/ProgramInfo.sol#L18

In reality, the timestamp in versionStarted and the start variables can be different. If the product is settled much later than the start in ProgramInfo, the timestamp of versionStarted will be much later than start.

    versionStarted = _start(self, programId, currentOracleVersion);

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

The same is also true for the timestamp in versionComplete and the start + duration in ProgramInfo. If the product is not settled frequently, the end of the program will be the product’s latest version, which is earlier than the start + duration.

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

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

The consequence is that a user who has a position in all the duration from start to start + duration may receive less rewards because the duration from versionStarted to versionComplete is shorter than the deserved duration that user should enjoy the reward. The inactiveDuration will be longer and users will have less rewards.

    uint256 inactiveDuration = programInfo.duration - (toOracleVersion.timestamp - fromOracleVersion.timestamp); 
    UFixed18 refundAmount = programInfo.amount.sum().muldiv(inactiveDuration, programInfo.duration);

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

Impact

Users can receive less rewards due to mismatched start and end time of the program.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L8-L20 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/interfaces/types/ProgramInfo.sol#L18 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/ProductManager.sol#L85 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L63 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L69-L70

Tool used

Manual Review

Recommendation

In determining the start and end of a program:

arjun-io commented 1 year ago

Tying incentive programs start and end time to actual settlements is necessary in order to have correct accounting. If the times are floating there are issues with share attribution

KenzoAgada commented 1 year ago

Closing as invalid. The impact is not substantial, and seems like there is no viable alternative to the implementation with the current Perennial mechanism.

thangtranth commented 1 year ago

Escalate for 10 USDC

Thank you sponsor and lead judge for your comments. However, I think the current implementation is not accounting for users correctly. As shown in my analysis above, here is a scenario:

t0-->t1-->t2-->t3

Users who have positions from t0 to t3 can only receive rewards from t1 to t2. The rest will be refunded.

The impact can be significant since it depends on the program total reward amount and the gap between the actual campaign start/end date and the current implementation.

The time is not floating since it can be clearly defined between start and start + duration variables.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Thank you sponsor and lead judge for your comments. However, I think the current implementation is not accounting for users correctly. As shown in my analysis above, here is a scenario:

t0-->t1-->t2-->t3

Users who have positions from t0 to t3 can only receive rewards from t1 to t2. The rest will be refunded.

The impact can be significant since it depends on the program total reward amount and the gap between the actual campaign start/end date and the current implementation.

The time is not floating since it can be clearly defined between start and start + duration variables.

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 commented 1 year ago

Few points:

Taking these into account, at the moment I would maintain issue invalidity.

thangtranth commented 1 year ago

I apologize for misunderstanding the meaning of “floating” as mentioned by the sponsor.

So the current implementation is: "the intended behavior of the reward program oracle versions is that the start oracle version will be the first oracle version that occurs after the start time, and the complete oracle version will be the last oracle version settled before the end time"

However the correct calculation should be: "the start oracle version will be the nearest oracle version after the start time, and the complete oracle version will be the nearest oracle version before the end time".

Let's review the versionStarted, now we only calculate in oracle version, not timestamp:

Let's say:

The versionComplete is also assigned as product.latestVersion() (which is the last time settle function was called and it can have a huge gap till the nearest version before completion). It may not be the nearest version before completion. https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/incentivizer/types/Program.sol#L63

Therefore, in the current implementation, the amount of incentive for a user depends not only on how long the user holds the position, but also on whether any account calls Product.settle. The later settle is called, the lower the rewards, which is clearly inaccurate.

KenzoAgada commented 1 year ago

In a good spirit, I think we can find some humor in this - This issue and #197 were similar - Sponsor disputed both of them separately - Then #197's escalation described the impact like this issue, and this issue's escalation described the impact like #197 🙂

jacksanford1 commented 1 year ago

Seems like the impact is a very small loss of rewards (assuming the issue is true)? This would not constitute a valid issue. @thangtranth

thangtranth commented 1 year ago

Hi @jacksanford1, the impact depends on two factors:

The impact can be significant for users if the reward program is very large and the settle function is called very late for any reason in a short program.

I agree that an issue is invalid if the loss is very small due to unavoidable reasons (for example, rounding from 1e18 to 1e6 in calculation for some tokens). However, this is an issue in the calculation logic and the impact can vary from very small to very large. Therefore, I think it is a valid issue.

arjun-io commented 1 year ago

This is the same issue noted as 4.1.5 in the Veridise audit, correct? We've also clarified in 4.1.6 in that audit why this accounting logic is necessary for proper accounting.

thangtranth commented 1 year ago

4.1.5 only addresses a part of this issue. This issue shows that both the start and the end of the program can be recorded incorrectly (t1 and t2 in the above example).

And the 4.1.6 states that “if the elapsed time is larger than the duration of the program, then there will be a revert due to subtraction overflow.” I also disagree with this finding because the elapsed time is shorter than duration due to the way it is recorded, not longer.

My suggestion is mentioned in the escalation paragraph:

However the correct calculation should be: "the start oracle version will be the nearest oracle version after the start time, and the complete oracle version will be the nearest oracle version before the end time".

jacksanford1 commented 1 year ago

Ok, this cannot be a High since it's a potential loss of rewards, not principal. But it could be a valid Medium.

Will let @arjun-io respond to @thangtranth's latest comment. And maybe @KenzoAgada has an updated take.

arjun-io commented 1 year ago

Our opinion for all of these incentivization start and end timestamp issues is that the incentivization start and end points are a matter of design decisions. The current implementation is well tested and correctly handles the indeterminate nature of oracle timestamps.

Without clear test cases/examples showing why one design choice is strictly superior to the current implementation (in that it correctly accounts for all cases and does not introduce other bugs) I struggle to see how these types of reports can be considered issues.

For example, in this specific recommendation, backdating the "start" doesn't work because the positions that apply at that version have already been settled so that version (and each individual account that has been settled) would need to somehow be re-processed to correctly sync the incentivizer.

thangtranth commented 1 year ago

It is my responsibility to show details and examples of how the current implementation has a potential loss of rewards. In the end, it is up to the protocol to decide what to do with it and what additional mechanisms to put in place to minimize the risk.

I leave it to Sherlock and the lead judge to make the final decision.

Thank you.

jacksanford1 commented 1 year ago

Really appreciate the detailed comments @thangtranth. And I think this issue is good to know about. However, due to the fact that it's only rewards at risk, the difference in owed vs. paid rewards will likely not be super large (in most cases) and the owner of the excess rewards is likely to be the program manager (I believe) there is a low likelihood that a malicious actor will be able to run off with these funds. Considering this issue Invalid.

jacksanford1 commented 1 year ago

Result: Invalid Unique Invalid due to previous comment's reasoning.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: