zzykxx - Auction rounds cannot be closed if the fee distribution call reverts

[sherlock-admin2]

zzykxx

medium

Auction rounds cannot be closed if the fee distribution call reverts

Summary

Auction rounds cannot be closed if distribute() reverts.

Vulnerability Detail

The function _closeAuction() transfers ETH fees to creator circle by calling the distribute() function:

// Distribute fee to beneficiary
if (l.highestBids[tokenId][currentAuctionRound].feeAmount > 0) {
    IBeneficiary(address(this)).distribute{
        value: l.highestBids[tokenId][currentAuctionRound].feeAmount
    }();
}

distribute() calls the out-of-scope IDABeneficiaryInternal::_distribute() function, which performs two other external calls:

l.token.upgradeByETH{ value: value }();
l.token.distribute(0, value);

There are some issues in this flow that might make the call revert preventing the auction from being closed, new rounds from starting and the NFT from being transferred:

• Both external calls are performed on l.token which implements the upgradeByETH() function but doesn't implement the distribute() function, making the call revert and bricking the auction process. (This can be verified in the ISETH interface of which l.token is an instance).
• Superfluid uses upgradable proxies in their contracts, meaning the smart contract code could be changed at any time making these functions revert for any new reason (the README specifies that external protocols are RESTRICTED).

Impact

Auction rounds might be bricked because of reverts on the distribute() call, preventing the auction from being closed, new rounds from starting, the NFT from being transferred and the auction fee from being distributed.

Code Snippet

Tool used

Manual Review

Recommendation

Wrap the distribute() call in a try/catch, cache the fee amount to be distributed when the catch is triggered and add a function to manually distribute the cached amount.

[zzykxx]

Escalate

Not a duplicate of #111. This issue is about an auction potentially being bricked if the distribute() call reverts. #111 is about not checking the return value of the distribute() function, but the distribute() function either reverts or returns true, which makes it invalid.

[sherlock-admin2]

Escalate

Not a duplicate of #111. This issue is about an auction potentially being bricked if the distribute() call reverts. #111 is about not checking the return value of the distribute() function, but the distribute() function either reverts or returns true, which makes it invalid.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Hash01011122]

@zzykxx Would you mind to please elaborate more on how its root cause is different from #111?? And incase #111 is invalidated why this should be a valid finding

[zzykxx]

111 is about the distribute() function potentially returning false. If it were to return false the protocol would continue to operate normally leaving the funds stuck, because it never checks for this possibility. The thing is that function distribute() either reverts or returns true, it never returns false. One argument in favor of keeping #111 valid is that the distribute() function technically returns a boolean and even if at the current state it never returns false it might do that in the future because the contracts are upgradable (keep in mind admins of external protocols are RESTRICTED for this audit).

This issue is about distribute() reverting and bricking the auction process.

[Al-Qa-qa]

Hi @zzykxx @Hash01011122,

I agree with most of the points you said @zzykxx.

I got the context of the distribution function wrong.

Once I saw it return bool, ERC20 transfer functionalities just jumped into my mind, and I thought that this value is used to check for the success of the distribution.

The feeAmount can get locked if the distributing function fails.

But this is not true, It will revert as you and other wardens explained. And unfortunately, I couldn't find the context of the function when auditing to know what it actually does.

---

In brief:

• You explained that if the distribution failed, the function will revert and the auction will be unclosable, and this is what will happen.

• I said that if the distribution fails, the function will not revert and the feeAmount will get locked in the contract, and this will not happen, but may occur in the future if the distribution mechanism changes as you pointed out.

Since I do not know how Judging and duplications work. I will not comment on the issue's validity and leave it to the Lead Judger.

[Hash01011122]

I agree with the escalation this issue should be de-duplicated from #111 and should be a valid high as it breaks core functionality of protocol. Let me know what do you guys think @gravenp @St4rgarden .

[zzykxx]

I agree with the escalation this issue should be de-duplicated from #111 and should be a valid high as it breaks core functionality of protocol. Let me know what do you guys think @gravenp @St4rgarden .

If this issue it's kept as valid it should be a medium severity issue instead of high, the core functionality is broken but there is no loss of funds.

[St4rgarden]

As the beneficiaries can be changed; is there was a malicious or inept contract beneficiary - there's no actual risk of bricking the contract or permanent loss of funds. So I don't believe this is an issue.

[Hash01011122]

@St4rgarden can you give a brief on why don't you see this as valid finding with appropriate reasoning, because to me its atleast suffice medium severity.

[realfugazzi]

The issue is highly speculative, there's no reason to believe a revert will be raised.

Both external calls are performed on l.token which implements the upgradeByETH() function but doesn't implement the distribute() function, making the call revert and bricking the auction process. (This can be verified in the ISETH interface of which l.token is an instance).

How this is not implemented? https://github.com/superfluid-finance/protocol-monorepo/blob/dev/packages/ethereum-contracts/contracts/apps/SuperTokenV1Library.sol#L1307

You can't simply speculate a malicious/bad distributor will be setup. Imagine raising an issue if every possible reference address in a protocol could be susceptible of not implementing their interface, every call will be a DoS.

Superfluid uses upgradable proxies in their contracts, meaning the smart contract code could be changed at any time making these functions revert for any new reason (the README specifies that external protocols are RESTRICTED).

This is completely out of scope/rules.

https://docs.sherlock.xyz/audits/judging/judging

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

[zzykxx]

@realfugazzi is correct.

• Both external calls are performed on l.token which implements the upgradeByETH() function but doesn't implement the distribute() function, making the call revert and bricking the auction process. (This can be verified in the ISETH interface of which l.token is an instance).

This claim is wrong, I missed that the l.token instance is using the SuperTokenV1Library.sol library. This was the main reason I submitted the report, I don't see any way the function would normally revert and even I did it's not described in the report.

This issue is invalid according to the rules. I would suggest the team to fix it with anyway as recommended, most of auction protocols have a way to gracefully handle a potential revert on auction closing.

[Evert0x]

Result: Invalid Unique

--

Thank you @zzykxx !

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• zzykxx: rejected