search

code-423n4 / 2022-09-artgobblers-findings

0 stars 0 forks source link

Possible centralization issue around RandProvider #327

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L560-L567

Vulnerability details

Impact

While it is very common for web3 projects to have privileged functions that can only be called by an admin address, special thought should be given to functions that can break core functionality of a project.

One such function is ArtGobblers.upgradeRandProvider(). If this is called passing a non-compatible contract address or EOA, requesting new seeds will be bricked and as a consequence reveals will not be possible. Also the seed could be controlled using a custom contract which would allow a malicious actor to set seeds in his favor.

Naturally, the assumption that the deployer or a multisig (likely that ownership will probably be transferred to such) go rogue and perform a malicious action is unlikely to happen as they have a stake in the project (monetary and reputation wise).

However, as this is a project that will be unleashed on launch without any further development and left to form its own ecosystem for many years to come, less centralized options should be considered.

Proof of Concept

The function ArtGobblers.upgradeRandProvider(), allows the owner to arbitrarily pass a RandProvider with the only restriction being that there is currently no seed requested from the current RandProvider:

function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
        // Revert if waiting for seed, so we don't interrupt requests in flight.
        if (gobblerRevealsData.waitingForSeed) revert SeedPending();

        randProvider = newRandProvider; // Update the randomness provider.

        emit RandProviderUpgraded(msg.sender, newRandProvider);
    }

The RandProvider is the only address eligible (as well as responsible) to call ArtGobblers.acceptRandomSeed(), which is required to perform reveals of minted Gobblers:

function acceptRandomSeed(bytes32, uint256 randomness) external {
        // The caller must be the randomness provider, revert in the case it's not.
        if (msg.sender != address(randProvider)) revert NotRandProvider();

        // The unchecked cast to uint64 is equivalent to moduloing the randomness by 2**64.
        gobblerRevealsData.randomSeed = uint64(randomness); // 64 bits of randomness is plenty.

        gobblerRevealsData.waitingForSeed = false; // We have the seed now, open up reveals.

        emit RandomnessFulfilled(randomness);
    }

This could be abused with the consequences outlined under #Impact.

Tools Used

Manual review

Recommended Mitigation Steps

The inclusion of a voting and governance mechanism should be considered for protocol critical functions. This could for example take the form of each Gobbler representing 1 vote, with legendary Gobblers having more weight (literally) based on the amount of consumed Gobblers.

Shungy commented 2 years ago

Similar but I don't think it is duplicate: https://github.com/code-423n4/2022-09-artgobblers-findings/issues/279 This finding only focuses on owner's ability to pause reveals. I think this should be lower severity as it is not as serious as "owner can instantaneously rug users at will" type of centralization.

GalloDaSballo commented 2 years ago

The warden has shown a few possible risks for end users, because of the privileged function upgradeRandProvider, a malicious owner could set the randProvider to either a malicious implementation or a faulty implementation.

This would prevent reveals which, as we know from other findings could cause the inability to mint legendary gobblers with non-zero emission factors.

Because this is contingent on a malicious Admin, which could deny reveals and hence deny a key aspect of the protocol, I believe Medium Severity to be appropriate

FrankieIsLost commented 2 years ago

We disagree with severity. This type of griefing attack by admin does not provide any economic benefits. Additionally, there doesn't seem to be any viable alternatives here, as introducing a governance system just to upgrade the rand provider (which should only happen once or twice during the lifetime of the project) seems like overkill

GalloDaSballo commented 2 years ago

I agree with the Sponsors unwillingness to create a complex system to maintain the VRF provider.

I also must concede that griefing the mint is of dubious economic benefit.

However, per our rules and historical context I believe this is an example of Admin Privilege, the Admin can change the implementation of the Randomness Provider to their advantage and can deny the mint from continuing.

I have to agree with a nofix, beside recommending the use of a strong Multisig to avoid any issues in the future.

However, the in-scope system has no way of:

C4 has historically flagged these type of risks as Medium, and for those reasons I believe the correct judgement is Medium Severity.

We will be discussing these types of findings to provide consistent and transparent judging rules in the future, however, given the historical track record of C4, I believe the right move is to keep it as Medium Severity

Pedroais commented 2 years ago

I agree with the Sponsors unwillingness to create a complex system to maintain the VRF provider.

I also must concede that griefing the mint is of dubious economic benefit.

However, per our rules and historical context I believe this is an example of Admin Privilege, the Admin can change the implementation of the Randomness Provider to their advantage and can deny the mint from continuing.

I have to agree with a nofix, beside recommending the use of a strong Multisig to avoid any issues in the future.

However, the in-scope system has no way of:

  • Ensuring a multisig will be used
  • Ensuring that a randomness provider which is fair is going to be used.

C4 has historically flagged these type of risks as Medium, and for those reasons I believe the correct judgement is Medium Severity.

We will be discussing these types of findings to provide consistent and transparent judging rules in the future, however, given the historical track record of C4, I believe the right move is to keep it as Medium Severity

For the judge's consideration I will argue why I think this issue should be downgraded to QA :

It's true that code4rena historically flags rug pull vectors as medium severity findings but in this case there isn't a stealing vector. There is no incentive for the owner to grief his own contract without any economic benefit to himself and with the drawback of ruining his own reputation and project.

Also many historical precedents judge these type of issues as informational or low severity findings.

Examples :

Centralization issue (informational): https://code4rena.com/reports/2022-08-prepo/#info-03-centralization-problems-in-vesting-contract

An issue that incurs economic loss but is very improbable (like an owner griefing his own project without any incentives): https://code4rena.com/reports/2022-08-nounsdao/#l01--nouns-will-not-be-able-to-be-transferred-once-the-blocknumber-passes-typeuint32max

In any case, I think it's reasonable to think of 3 different scales of severities :

I think it would be quite common sense to get this issue a lower severity than a "rug pull vector" type of issue. Also while code4rena contest work on rules and shouldn't be arbitrary, I think there is always edge cases in judging that require the judge to use his discretion and common sense, not based on payout but based on the relative severity of different issues that are being graded the same severity.

Regards, pedro

IllIllI000 commented 2 years ago

A founder may have a fight with a team member and may brick things out of spite. A founder may be paid off by a competing company in the same space. A founder may decide that they 'deserve' more money and decide to short their tokens on an exchange before bricking things. A founder may take pleasure in the suffering of others.

Minh-Trng commented 2 years ago

There is no incentive for the owner to grief his own contract without any economic benefit

there is no instantaneous economic benefit as in a rug pull, but being able to control the seeds also means being able to control the reveals. If that can be pulled of unnoticed one could get all the higher emmission gobblers, which are inherently more valuable, but also equate to owning a higher percentage of the Goo supply.

Edit: just as a disclaimer, I am the one who submitted the issue, so naturally the opinion above is biased

Pedroais commented 2 years ago

A founder may have a fight with a team member and may brick things out of spite. A founder may be paid off by a competing company in the same space. A founder may decide that they 'deserve' more money and decide to short their tokens on an exchange before bricking things. A founder may take pleasure in the suffering of others.

Those things are improbable, the issue should be low severity vs a regular rug pull. You could say that exact thing of any only owner function in any protocol. Your argument is unserious.

there is no instantaneous economic benefit as in a rug pull, but being able to control the seeds also means being able to control the reveals. If that can be pulled of unnoticed one could get all the higher emmission gobblers, which are inherently more valuable, but also equate to owning a higher percentage of the Goo supply.

Blockchain is open can't go unnoticed, it would grief the project and not benefit the owner since the project would lose it's value.

Any only owner function could be used to grief if the owners start doing things against the protocol. Also owners can grief a project off chain (killing it, pulling the frontend off, etc...). If we follow your logic all only owner functions should be medium issues since they can be used against the protocol. I argue only stealing , rug pull types of owner privilege issues should be medium severity.

IllIllI000 commented 2 years ago

Those things are improbable, the issue should be low severity vs a regular rug pull. You could say that exact thing of any only owner function in any protocol. Your argument is unserious.

You're welcome to your opinions, but the opinions you're stating are not facts. I'm being serious and don't appreciate the condescension. If an owner is going to rug, they're going to find a way to rug - it's just a matter of how much work they have to put into it. Isn't 'say[ing] that exact thing of any only owner function', exactly what we're discussing here?

Pedroais commented 2 years ago

Those things are improbable, the issue should be low severity vs a regular rug pull. You could say that exact thing of any only owner function in any protocol. Your argument is unserious.

You're welcome to your opinions, but the opinions you're stating are not facts. I'm being serious and don't appreciate the condescension. If an owner is going to rug, they're going to find a way to rug - it's just a matter of how much work they have to put into it. Isn't 'say[ing] that exact thing of any only owner function', exactly what we're discussing here?

Of course they're not facts we're all stating our opinions respecting severities.

Your opinion is all only owner functions that adjust protocol parameters and can therefore be used to the protocol's prejudice should qualify as medium severity issues even if they don't let the owner steal value from the protocol(please tell me if I'm misrepresenting what you said, not my intention). The owner is even counter-incentivized to do it since they probably have an initial allocation and would grief their own protocol while not stealing anything. Any protocol owner could short the protocol and grief it but are counter-incentivized to do it since it's their project and they have skin in the game.

My opinion is owner stealing should be medium and owner griefing should be low, for obvious reasons of incentives and probability.

Ultimately the judge will make a decision and I've deployed all my arguments already so won't answer anymore.

regards, it wasn't my intention to offend you

IllIllI000 commented 2 years ago

I have avoided providing an opinion on whether it should be low or medium, since I can see both sides. I filed the issue as medium, because I know it's usually marked as medium. People offer up opinions and I give counter-arguments until I feel I have a full picture. @GalloDaSballo is there a reference for the historical reason for Medium? I remember that there were discord discussions when I first joined, about something similar, but I don't know whether that's where it started, or if there was something else

GalloDaSballo commented 2 years ago

@Pedroais while your argument superficially makes sense, there are always ways to make Admin Privilege profitable in the broader context of the market, the world, and personal interest.

Also, see below a list of 30 times in which a finding similar to this one was flagged as Medium Severity (you sent one of them btw): https://gist.github.com/GalloDaSballo/881e7a45ac14481519fb88f34fdb8837

I'd like to emphasize that out of the 30 Admin Privilege flags, 1 did end up abusing the power and did steal funds from the users.

I had written a long list of possible attacks we can imagine from this form of Admin Privilege. However, I don't believe the Sponsor needs to be subject to this discussion.

I understand the sponsor is acting in good faith and appreciate their patience.

We can continue the discussion on this issue: https://github.com/code-423n4/org/issues/59

For this specific finding, there is a clear risk of persistent Denial of Service at the exclusive privilege of the Owner, so it's my job to flag it as Medium until the rules are changed.

Feel free to flag this to the org and hopefully we can have a productive discussion in the future.

I believe the rules can be changed if the majority decides to do so. However, it would be incorrect for me to change my mind at this time.