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

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

Unbounded Growth in Portal Energy #52

Open hats-bug-reporter[bot] opened 11 months ago

hats-bug-reporter[bot] commented 11 months ago

Github username: @hama-tech Twitter username: -- Submission hash (on-chain): 0x589ff31d64ed9fac8e4d56dc42d2444245bc4a3f5dc172ecd80157976a3d67e4 Severity: high

Description: Description\

PortalEnergy has value as it can be sold for Possum(PSM) tokens via sellPortalEnergy. One way a user's PortalEnergy gets updated is in the _updateAccount function. A user's portalEnergy gets incremented by portalEnergyEarned+portalEnergyIncrease: https://github.com/PossumLabsCrypto/Portals/blob/5e1855411121ccd883f15c0d3c8d2fd9fc1d8e4c/contracts/Portal.sol#L205

function _updateAccount(){
   uint256 portalEnergyEarned = (accounts[_user].stakedBalance * 
  (block.timestamp - accounts[_user].lastUpdateTime)) / SECONDS_PER_YEAR;

 ...
    accounts[_user].portalEnergy +=
    portalEnergyEarned +
    portalEnergyIncrease;
 ...
}

This issue describes how a user can infinitely increase his portalEnergy ,once he stake a big amount ,through the portalEnergyEarned variable by staking a very low amount each time (1 wei).

portalEnergyEarned is calculated using accounts[_user].stakedBalance (the users s old staked balance each time it s called) .

The accounts[_user].portalEnergy s increased by portalEnergyEarned value calculated

for example : setting ((block.timestamp - accounts[_user].lastUpdateTime) / SECONDS_PER_YEAR) as a short constant interval of time named _timeRatio

alice staked a big amount of tokens in wei named _stakedAmount

that her portalEnergyEarned equals 10.

portalEnergyEarned = _stakedAmount * _timeRatio

alice can get in a loop of staking 1wei that _stakedAmount =_stakedAmount +1 to get another 10 portalEnergyEarned (slightly more). Thus alice can cause financial loss to the contract as she can call sellPortalEnergy for almost all PSM available in contract, and then unstake.

Attack Scenario

Initial Large Stake: A user stakes an amount larger than the number of seconds per year, resulting in a non-zero portalEnergyEarned based on the time elapsed since the last update.

Repeated Small Stakes: The malicious user calls the _updateAccount function with a very small amount multiple times, causing the portalEnergyEarned to increase continuously.

Unbounded Increase: Due to the repeated small stakes, the portalEnergy for the user grows without practical limits, leading to an unrealistic increase in portal energy.

Withdrawal Exploitation: Since User has been able to increase his portalEnergy to any value, he can sellPortalEnergy for almost all PSM available in contract, and then unstake.

Attachments

  1. Proof of Concept (PoC) File

See steps in attack scenario

  1. Revised Code File (Optional)

Maximum Cap on portalEnergyEarned: Introduce a maximum cap on portalEnergyEarned in each function call. Limit the increase in portalEnergyEarned to a predefined maximum value, preventing unbounded growth.

Periodic Reset or Decay: Implement a mechanism to periodically reset or decay the portalEnergy values, ensuring that the growth is controlled and does not lead to impractical results.

PossumLabsCrypto commented 11 months ago

Thanks for the submission! Unfortunately, this is a double from #21 / #45

While in your POC the attacker would call stake() instead of unstake() to exploit the rounding error in _updateAccount(), it is the same underlying vulnerability.

Great find nonetheless! :)

hama-tech commented 11 months ago

Hey sir thanks for your reply . the bug i described may share the same impact as the bugs you refereed to, but still not the same cause of what will result int the inflation of portalEnergy earned. for the refference to the bug #45 it s about rounding error, my bug has nothing to do with rounding as it s all focused about the multiplication happening here

portalEnergyEarned = (accounts[_user].stakedBalance * 

(block.timestamp - accounts[_user].lastUpdateTime)) / SECONDS_PER_YEAR

for the bug #21 it uses the same attacking mechanics for the unstake function but due to different cause and different impact which it cause the status update of global variables to be out of sync.

for my bug again it will be exploited thro staking a big amount at first, then keep calling stake with small amounts to keep incremanting accounts[_user].portalEnergy with portalEnergyEarned thats calculated with accounts[_user].stakedBalance that was a big amount at the begining and keep increasing with small amounts later. Thus for every small amount staked user gets big amount of portalEnergyEarned based on the old big staked value. This which will later be traded back with PSM tokens.

i will apreciate it if you reconsider my bug. thanks,

hama-tech commented 11 months ago

the idea of this bug that the user would still get an amount of portal energy tokens as if he staked a big amout of PSM each time he calls stake(). although he just stakes that big amount at the beginning, and just keep staking small amounts of PSM so he gets the same amount of portal energy tokens as if he staked the same big amout of PSM based on this calculation uint256 portalEnergyEarned = (accounts[_user].stakedBalance * (..; .. accounts[_user].portalEnergy += portalEnergyEarned + ..;

PossumLabsCrypto commented 11 months ago

Thank you for highlighting the difference in your report and the additional context. 👍

However, what you describe is not an exploit but the intended functionality. Users earn Portal Energy on a continuous basis and this is recorded in their struct whenever _updateAccount() is called. Because of solidity´s way of rounding down at the precision limit, your proposed attack would in fact just hurt the attacker as they earn less Portal Energy than they would if they waited for longer time.

although he just stakes that big amount at the beginning, and just keep staking small amounts of PSM so he gets the same amount of portal energy tokens as if he staked the same big amout of PSM based on this calculation

This is how it´s supposed to be. All staked tokens continue to earn Portal Energy every second.

Unless I completely misunderstood your submission, this finding is invalid.

Please let me know if that clarified the mechanism. 🙂

hama-tech commented 11 months ago

Sir on the contrary the attacker won't be waiting , after he makes the 1st big stake he will be calling stake() with small amounts continuously every short period so he keeps doubling on accounts[_user].portalEnergy as it increases here

accounts[_user].portalEnergy +=    portalEnergyEarned +   portalEnergyIncrease;

portalEnergyEarned is mainly based on his first big stake

portalEnergyEarned = (accounts[_user].stakedBalance * (block.timestamp - accounts[_user].lastUpdateTime)) / SECONDS_PER_YEAR;

as i mentioned in my report

 setting ((block.timestamp - accounts[_user].lastUpdateTime) / SECONDS_PER_YEAR) as a short constant interval of time named _timeRatio

 alice staked a big amount of tokens in wei named _stakedAmount that her portalEnergyEarned equals 10.

 portalEnergyEarned = _stakedAmount * _timeRatio

 alice can get in a loop of staking 1wei that _stakedAmount =_stakedAmount +1
 to get another 10 portalEnergyEarned (slightly more).

thus after 10 stakes _stakedAmount =_stakedAmount +10 and portalEnergyEarned would be added 10 times that accounts[_user].portalEnergy += 100 + portalEnergyIncrease;

in a very short time due to her just calling on stake() again based on her 1st big stake .

PossumLabsCrypto commented 11 months ago

To clarify, when you say the attacker does 10 stakes in "very short time" do you mean within the same block or over one / several blocks?

How exactly is this loop of 10 staking actions executed?

PossumLabsCrypto commented 11 months ago

My bad, it doesn´t even matter if the loop happens in one block or multiple.

Every execution of _updateAccount() also updates accounts[_user].lastUpdateTime = block.timestamp; This prevents any following call in the same block to earn any Portal Energy for the stake because the time difference is zero in

        uint256 portalEnergyEarned = (accounts[_user].stakedBalance * 
            (block.timestamp - accounts[_user].lastUpdateTime)) / SECONDS_PER_YEAR;

If the call happens over multiple blocks, the rounding down of solidity ensures that the "attacker" doesn´t earn Portal Energy that he would earn when waiting longer.

This is an invalid finding.

hama-tech commented 11 months ago

Sir here is a numeric example to clarify this: for block.timestamp - accounts[_user].lastUpdateTime being equals to 10hour, 36000 s SECONDS_PER_YEAR is 31536000 s if alice stakes at first 10 000 this equation

 portalEnergyEarned = (accounts[_user].stakedBalance * 

(block.timestamp - accounts[_user].lastUpdateTime)) / SECONDS_PER_YEAR;

would be

 11.4155251142 = 10000 * (36000) / 31536000

the attacker can call stake() every 10hours to get 11 portalenergy earned. when he calls it again with staking one token it would be calculated like this:

    11.416666666  = 10001 * (36000) / 31536000

and accounts[_user].portalEnergy += portalEnergyEarned ; will be increased like that every 10 hours with just staking one token at a time. If the attacker staked 100 000, he can get the same amount every 1 hour. Please sir reconsider this.

PossumLabsCrypto commented 11 months ago

As mentioned before, that is exactly how the system is supposed to work. Portal Energy increases every second depending on the total amount staked by the user. It doesn´t matter how often the struct is updated via _updateAccount().

hama-tech commented 11 months ago

yea it increases due to a ratio, but this way it gets doubled each times there s a stake.

PossumLabsCrypto commented 11 months ago

Sorry but I don´t see a calculation that would support that claim. There is no doubling.

hama-tech commented 11 months ago

sir this bug seems like the one described here #5 tho the cause here is portalEnergyEarned.