bitsurfer - Perennial (Incentivizer) Reward Token open to decimal issue

[sherlock-admin]

bitsurfer

medium

Perennial (Incentivizer) Reward Token open to decimal issue

Summary

Perennial (Incentivizer) Reward Token open to decimal issue

Vulnerability Detail

The Perennial reward token documentation mentions that it doesn't support non-standard ERC20 fee-on-transfer and rebase tokens. However, it doesn't explicitly state whether it only supports 18 decimal token precision. (This reward token is different from Collateral token, which is USDC / DSU).

File: ProgramInfo.sol
10: struct ProgramInfo {
...
14:     /// @dev Amount of total maker and taker rewards
15:     Position amount;
...
23:     /**
24:      * @dev Reward ERC20 token contract
25:      * @notice Perennial does not support non-standard ERC20s as reward tokens for incentive programs, including,
26:                 but not limited to: fee on transfer and rebase tokens. Using such a non-standard token will likely
27:                 result in loss of funds.
28:      */
29:     Token18 token;
30: }

The amount of rewards also measured in Position, which is an UFixed18 amount for maker and taker.

File: Position.sol
12: struct Position {
13:     /// @dev Quantity of the maker position
14:     UFixed18 maker;
15:     /// @dev Quantity of the taker position
16:     UFixed18 taker;
17: }

Both token and amount of Incentive reward is stated 18 decimals. Issue arise when Product owner or Program Manager using a token with decimal not 18, like USDC (6 decimal).

for example, when creating a program, by calling Incentivizer:create(), and its part of code:

File: Incentivizer.sol
56:         // Take fee
57:         UFixed18 programTotal = programInfo.amount.sum();
58:         UFixed18 programFeeAmount = programInfo.deductFee(_controller.incentivizationFee());
59:         fees[programInfo.token] = fees[programInfo.token].add(programFeeAmount);
60:
...
64:         // Charge creator
65:         programInfo.token.pull(msg.sender, programTotal);

This programTotal will sum up (maker & taker) amount with 18 decimal precision, at the end to charge creator, it will pull (line 65) (transfer in) the reward token with 18 decimal precision, even though it's a 6 decimal token. USDC with its 6 decimal token is a common stable coin which is being used widely, so assuming the reward token would be supporting USDC is an acceptable one.

Decimal precision is crucial when working with tokens, as it determines the level of granularity for representing fractional amounts. In this case, if the Perennial Reward token lacks proper decimal handling, it may result in incorrect reward calculations.

Impact

Lost decimal precision in reward token may result in incorrect reward calculations

Code Snippet

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

Tool used

Manual Review

Recommendation

Do not expect every Reward token would be a 18 decimal token (UFixed18). Provide a flexible approach to handle reward tokens with varying decimal precision

[KenzoAgada]

While true, the comments do mention not all ERC20 will be supported, not limited to rebase tokens etc'. This is valid but I'm not sure if a medium severity is justified... Judging as valid low, as the protocol might want to add to comments/documentation that different decimals are not supported.

[arjun-io]

We'll add a comment here clarifying decimal requirements

[sherlock-admin]

Escalate for 10 USDC

First, I hope @hrishibhat, @Evert0x, or @jacksanford1 can be the middle man here.

My finding about the incentivizer token reward decimal an issue, is indeed a valid one, maybe my explanation is not exaggerating it very well, but it definitely a medium. The reward calculation topic is an issue which should be a medium, if not high, due to potential loss of funds.

In my thinking:

1. because the issue is related with a simple overlooked decimal issue (not a complicated one like others), the weight of judgement seems to devaluate the issue, while in fact, it can lead to potential loss of funds.

2. because the word "not limited" then judge devaluate this finding to low, IMHO, is dwarfing the meaning of this issue, I mean, if I didn't highlight this issue out, most people (and maybe including perennial dev) might actually think:

   For Incentivizer reward token, the rebase, fee-on-transfer are not supported and also perhaps other weird ERC20, but for sure USDT or USDC, top stable token will be accepted because it's a common token for a reward (Even sherlock paid the contest reward in USDC, not SHER :)

File: ProgramInfo.sol
23:     /**
24:      * @dev Reward ERC20 token contract
25:      * @notice Perennial does not support non-standard ERC20s as reward tokens for incentive programs, including,
26:                 but not limited to: fee on transfer and rebase tokens. Using such a non-standard token will likely
27:                 result in loss of funds.
28:      */
29:     Token18 token;

I can argue, most people will think the Perennial does not support non-standard ERC20s as reward tokens for incentive programs, including, but not limited to words in the beginning of notice, will exclude the popular USDC or USDT

By all means, if team protocol at the beginning knows about the issue which they can only process 18 decimals token, they must have put the info in place, even before mentioning the rebase and fee-on-transfer token. Even if Perennial actually intended to only accepting DSU as reward token, then it should mention it only accept DSU, and not accepting a 'custom' reward token.

---

.. Judging as valid low, as the protocol might want to add to comments/documentation that different decimals are not supported.

If Perennial updating their rules by saying it will only accepting 18 decimals due to my finding, which previously can potentially cause lost of asset, and my submission just marked as low without any payout, seems off and unfair.

The "not limited" words here should not be a way to escape and disincentivize my valid finding.

If my finding being rejected because it related to how some Weird ERC20 token will not work in this reward then I can accept that argument, but not this one.

---

I apologize if my argument for escalation seems lengthy, but I find it unfair that the reason for marking this issue as low is the "not limited". Maybe, if this issue had been submitted by a Senior Watson, it would have undoubtedly been accepted without debate or the need for escalation.

You've deleted an escalation for this issue.

[securitygrid]

Comment from a waston: It is recommended that you cancel the escalate to save $10. The definition of ProgramInfo.token is Token18.

[0xbitsurfer]

Comment from a waston: It is recommended that you cancel the escalate to save $10. The definition of ProgramInfo.token is Token18.

Wow, thanks for the heads up @securitygrid, really appreciate it! I don't mind about saving the $10 on escalation, but what made me retract my escalation is your simple explanation of The definition of ProgramInfo.token is Token18. which is an overlooked on my end, I admit it.

The judge and Perennial dev reasoning made me want to escalate. but your simple reason, made me feel dumb after writing the long escalation message, as it is now became a user input validation issue. Learn my lesson here.

Thanks @securitygrid !