search

hats-finance / Possum-Labs--Portals--0xed8965d49b8aeca763447d56e6da7f4e0506b2d3

GNU General Public License v2.0
0 stars 2 forks source link

Missing Return Minted Tokens Handling in Deposit to HLP_STAKING #73

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x2d6273057eaf538513d62b84b6bb98f9ad06dafb1eaa5f609172b1333c4c6c7a Severity: medium

Description: Description\ Currently, when you deposit to HLP_STAKING, it doesn't handle or return any token. The problem arises if HLP_STAKING is tokenized and decides to mint tokens for depositors. In the current situation, you deposit HLP, and the contract mints sHLP for the contract. As the convert function can be called by anyone, a malicious user could potentially obtain all sHLP tokens.

Attack Scenario\ A malicious user exploits the lack of return value handling in the deposit to HLP_STAKING, potentially draining the contract and preventing users from unstaking.

Impact\ A malicious user could drain the contract, and users would not be able to unstake.

Label\ The bug was found in Pickle Finance. https://youtu.be/pJKy5HWuFK8?t=1688 the impact was low in Pickle Finance because only trusted authority could transfer tokens but here as anyone could call the convert function, it has a high impact. As the likelihood of this issue is low, like #39 issue, I labeled it as a medium.

Attachments\

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

    One possible solution is to add a list of tokens that couldn't be transferred in the convert function, and a trusted authority can update the list. This would prevent unauthorized access to newly minted tokens.

cpp-phoenix commented 1 year ago

It is somewhat similar to Upgradable contracts which is considered out of scope for this audit. You can take a look at this similar finding https://github.com/hats-finance/Possum-Labs--Portals--0xed8965d49b8aeca763447d56e6da7f4e0506b2d3/issues/59

PossumLabsCrypto commented 1 year ago

Interesting idea.

However, as @cpp-phoenix mentioned, this attack requires access to a trusted address (proxy) and is out of scope.

Adding an blacklist to the convert() function effectively grants power over a core economic function which is a central point of failure. This control is so significant that it could be used to destroy the Portal.

This does not match our design approach of building credibly neutral protocols without any admin access and in this case we rather accept the risk of the underlying protocol making arbitrary changes. Generally, we seek to avoid these risks in the future by not building on top of upgradable proxies where possible but for now it is what it is.