The Standard

Summary

Inside LiquidationPool a user can decrease his staked amount, which check whether his stake is 0 and if it is, he is removed from holders array. The problem arrises from the parallel structure pendingStakes , which hold pending stakes for the users.

Vulnerability Details

If a user has a position of 1000 staked TST and increment it with another 1000, second 1000 TST would be queued in pendingStakes and would be added to user’s position after 24 hours. If the same user decide to decrease his position with 1000 TST, this would remove him from holders array, because the check only look inside position mapping for the given user:
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);

But when the period of 24 passes positions[user] will be filled with his position, but the user won’t be presented inside holders array.

PoC

In the following gist I have provided coded PoC and instructions on how to run it.

https://gist.github.com/NicolaMirchev/b50b266f76c8efbc20420d5f71f5ffaa

Impact

• Calculation errors, due to getTstTotal, which iterates over holders array

• Missed rewards, because distributeFees & distributeAssets iterate over holders array

Tools Used

• Hardhat

• Manual Review

Recommendations

• One solution is to check pendingStakes for the user, before deleting him

• Another is to call addUniqueHolder inside consolidatePendingStakes , when a position is being incremented