Can inflate portalEnergy

[hats-bug-reporter[bot]]

Github username: @@ethanbennett Twitter username: @thre3th Submission hash (on-chain): 0x5f396daec99d26c9231dab695b2e7496ff7844d490474acf084f07e1d4398812 Severity: high

Description: Description\ Due to rounding errors in the calculation of portalEnergy when a user unstakes, it is possible to unstake funds without the portal deducting the proper amount of portalEnergy.

Attack Scenario\ This can be exploited as seen in the attached PoC, where an attacker can use just 40 principal tokens to inflate their portalEnergy to 80, and then confirms that it has all the principal tokens and 80 portal energy tokens at the end of the transaction.

Attachments

1. Proof of Concept (PoC) File https://gist.github.com/ethanbennett/0b2c8e85089b89588eeebfc602cf0763

2. Revised Code File (Optional)

[ethanbennett]

A better title for this finding: Users can bypass portalEnergy calculations when unstaking to steal extra yield from the contract

In this comment I will expand on the mitigation, and in the following one, I'll explain the attack scenario and the impacts more clearly.

The simplest solution would likely be instituting a minimum for the _amount of principal tokens in both stake and unstake. Note that this minimum is a function of the maxLockDuration: it should not be referenced explicitly, since it will change with maxLockDuration (which, itself, is designed to change). As is, the minimum number of principal tokens with which the contract operates properly is 5.

You could check for this dynamically with:

require (_amount * maxLockDuration >= SECONDS_PER_YEAR)

The reason I suggest this over adjusting the math is that rounding up instead of down could present a different risk in this context. However, if it is important to retain this functionality for small _amounts, the math could be reworked instead.

[ethanbennett]

Expanding on the attack scenario: an attacker is able to inflate their portalEnergy infinitely by exploiting the rounding error on L314 of Portal.sol — since the division will round down, any _amount small enough to make _amount * maxLockDuration less than SECONDS_PER_YEAR will return 0. Any logic that uses the raw _amount proceeds as intended, so the attacker still receives their unstaked funds like they normally would. But they also keep all the portalEnergy they built up in the process.

It is possible that this could be exploited more efficiently or with greater impact than the loop in the PoC, but I believe this attack is already significant enough to consider it high-risk: portalEnergy has real value within Portal, and that value can be stolen without limit by any attacker.

[PossumLabsCrypto]

Thank you for the submission and thorough POC.

Unfortunately, this bug has been found here already: #21

You have clearly explored this vulnerability from an individual angle and given the severity I´d like to send you a guesture of appreciation to the wallet that has made this submission when the Portal launches. Though, being a double finding, it is excluded from competition rewards, thanks for understanding. 🤝

PS: To anyone reading this, it is the first and only time I´ll make such an exception. Don´t want to create skewed incentives here. 🙂

[ethanbennett]

@PossumLabsCrypto I appreciate the gesture and I do understand where you're coming from, but respectfully, I would like to make a case against discarding this as a duplicate.

21 is quite broad, and it mentions four variables that do not update when the operation rounds down — only one of these is relevant to the exploit I reported, and it's only a part of my report. The fundamental vulnerability in my report is an implicit minimum number of principal tokens that affects all of your accounting logic but is never accounted for in your code, and this is not expressed as such in #21 . That report certainly overlaps with mine, but they are not the same.

Consider also that my attack vector allows for inflating the portal energy beyond what should be possible for a given number of principal tokens, while #21 focuses on the inaccurate state update.

More significantly still, on #21 , you clearly stated that you did not consider their finding to be exploitable and you did not intend to fix it. If not for my report, the protocol would still have been vulnerable to this exploit and would have lost funds as a result. Such a finding should be rewarded in the framework of the audit, otherwise the incentive to report findings becomes moot.

Please also consider that my report includes a mitigation suggestion that does not require reworking the accounting logic of the entire contract, which you mentioned as the blocker to addressing #21 .

Despite similarities, mine is a huge vulnerability you would have shipped with even after reviewing the subtly different #21 .

[PossumLabsCrypto]

I understand your frustration for being late but I respectfully disagree. Your report targets exactly the same section of the code and outcome as #21 and is therefore a double.

As stated in issue 21, the code will not be fixed because this exploit is not economical when considering Tx costs. Aside, it is also self-healing because the maxLockDuration will increase to one year and more over time which eliminates this rounding issue. To be precise, 6 months after launch, this issue will not exist anymore even without changing the code.

My offer to reward you outside of the competition was made in good faith to signal appreciation for your hard work. If you think participating in a competition where the reward goes to first findings only makes it moot, I´m sorry to hear that but the rules have been communicated clearly in advance.

For reference, here is a side-by-side comparison of the reports. This is clearly the same issue in different words.

Wording of #21

First of all, the value of maxLockDuration is 7776000, and the value of SECONDS_PER_YEAR is 31536000. The value of the former is smaller than the latter. And 31536000 / 7776000 = 4.055555555555555. [...] This vulnerability allows users to keep the values of accounts[msg.sender].maxStakeDebt and accounts[msg.sender].portalEnergy unchanged while unstaking all their own tokens.

Your summary above:

an attacker is able to inflate their portalEnergy infinitely by exploiting the rounding error on L314 of Portal.sol — since the division will round down, any _amount small enough to make _amount * maxLockDuration less than SECONDS_PER_YEAR will return 0. Any logic that uses the raw _amount proceeds as intended, so the attacker still receives their unstaked funds like they normally would. But they also keep all the portalEnergy they built up in the process.

[ethanbennett]

I know your offer is in good faith and I absolutely appreciate it. Thanks for hearing my side and taking the time to respond.