Submitted by Jeiwan, also found by pashov, cryptonue, 0xbepresent, __141345__, unforgiven, cryptoDave, Koolex, and datapunk
A user (which can also be one of the autocompounding contracts, AutoPxGlp or AutoPxGmx) can lose a reward as a result of reward tokens mismanagement by the owner.
The protocol defines a short list of reward tokens that are hard coded in the claimRewards function of the PirexGmx contract (PirexGmx.sol#L756-L759):
The fact that these addresses are hard coded means that no other reward tokens will be supported by the protocol. However, the PirexRewards contract maintains a different list of reward tokens, one per producer token (PirexRewards.sol#L19-L31):
These reward tokens can be added (PirexRewards.sol#L151) or removed (PirexRewards.sol#L179) by the owner, which creates the possibility of mismanagement:
Such mismanagement can cause users to lose rewards for two reasons:
In the claim function:
In the above loop, there can be multiple reasons for rewards to not be sent:
In all of the above situations, rewards wont be sent, however users reward state will still be set to 0.
Also, notice that calling claim wont revert if reward tokens are misconfigured, and the Claim event will be emitted successfully, which makes reward tokens mismanagement hard to detect.
The amount of lost rewards can be different depending on how much GMX a user has staked and how often they claim rewards. Of course, if a mistake isnt detected quickly, multiple users can suffer from this issue. The autocompounding contracts (AutoPxGlp and AutoPxGmx) are also users of the protocol, and since theyre intended to hold big amounts of real users deposits (theyll probably be the biggest stakers), lost rewards can be big.
Consider having one source of reward tokens. Since theyre already hard coded in the PirexGmx contract, consider exposing them so that PirexRewards could read them in the claim function. This change will also mean that the addRewardToken and removeRewardToken functions wont be needed, which makes contract management simpler.
Also, in the claim function, consider updating global and user reward states only after ensuring that at least one reward token was distributed.
drahrealm (Redacted Cartel) disagreed with severity and commented:
Picodes (judge) commented:
For this contest, 60 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by 0xSmartContract received the top score from the judge.
The following wardens also submitted reports: brgltd,
Deivitto,
Awesome,
eierina,
jadezti,
0xNazgul,
adriro,
danyams,
delfin454000,
rotcivegaf,
sakshamguruji,
rbserver,
Waze,
Josiah,
hansfriese,
gz627,
oyc_109,
keccak123,
B2,
ch0bu,
0xbepresent,
cryptostellar5,
Diana,
Funen,
0xfuje,
pedr02b2,
nameruse,
deliriusz,
Jeiwan,
joestakey,
unforgiven,
xiaoming90,
shark,
erictee,
JohnSmith,
0xPanda,
btk,
0xAgro,
gzeon,
hihen,
carrotsmuggler,
R2,
subtle77,
codeislight,
simon135,
Rolezn,
aphak5010,
csanuragjain,
datapunk,
martin,
Sathish9098,
yixxas,
perseverancesuccess,
fatherOfBlocks,
rvierdiiev,
codexploder,
chaduke,
Bnke0x0, and
RaymondFam
.
