Post-judging QA

[CloudEllie]

🧑‍⚖️ Post-judging QA

1. Please post feedback about specific findings on the relevant issue in this repo.
2. More general feedback for the judge can be shared here.

The judge for this contest is @dmvt.

Reminders

• Please keep your comments as fact-based as possible.
• C4 judges have final authority in determining validity and severity.
• You are welcome to start new discussions here!
• Please link to specific issues wherever relevant.

Thank you!

[kirk-baird]

For #313 I don't believe this is valid as the tokens being deposited and withdrawn are Curve LP tokens and therefore a known token which is not vulnerable to reentrancy.

[dmitriia]

It is actually already stated in the first part of issue description. If LP tokens allowed reentrancy now it should be high/critical, not medium.

@dmvt What do you think?

[kirk-baird]

It is actually already stated in the first part of issue description. If LP tokens allowed reentrancy now it should be high/critical, not medium.

@dmvt What do you think?

I believe the correct way for judging reentrancy is non critical if the current token contracts are know to not be reenterable and then upgraded to low if they are upgradable.

[dmitriia]

In my opinion such reentrancy enabling logic should be addressed even when the probability of execution is low at the moment. It's an expectation, the probability multiplied by the impact, and the impact is full funds loss.

[kirk-baird]

I agree with you that is a serious issue. I rated some equivalent bugs where reentrancy is possible to drain all funds if the contract is upagraded to be reenterable. There was a lot of backlash from the community saying my issues should have been low 😳. One was in the trader Joe contest.

[dmvt]

It is actually already stated in the first part of issue description. If LP tokens allowed reentrancy now it should be high/critical, not medium.
@dmvt What do you think?

I believe the correct way for judging reentrancy is non critical if the current token contracts are know to not be reenterable and then upgraded to low if they are upgradable.

I agree with this. I'm aware that the curve contracts are upgradable, but I believe that the world where someone compromises the curve admin functionality and then uses that compromise to rug this protocol specifically through a reentrancy attack is very unlikely. It's much more likely, for instance, that the attacker would simply upgrade all of the curve pools to steal funds directly. Likewise it is very unlikely that curve will introduce a reentrancy attack into it's own protocol and Aura will wind up being the victim. If this were a less well known protocol without the battle tested history of Curve, I might rate this as higher, but in this case QA (aka low) makes sense to me.

[kirk-baird]

For issue #301

The first part execute() is only ever called from Booster.sol here where the to address is verified when set in poolInfo.

For CreateStash() case gauge is checked upstream for all the call paths. Thus this issue cannot be reached. See CreateStash.

• Checked PoolManagerProxy.addPool()
• Checked PoolManagerSecondaryProxy.addPool()
• Checked PoolManagerV3.addPool()

I don't believe this issue is valid as there is no exploit path as all to addresses are verified before making external low level calls.

[dmvt]

Great catch. I missed those additional guards in my review. Marking #301 invalid.

[kirk-baird]

I thought the process was that zero address checks are no longer Medium and considered QA #237 and duplicates of M-17?
Similarly aren't two stage ownership transfers considered QA e.g. #51 M-15?
Similarly aren't two stage update operations considered QA e.g. #31 M-04 and #32 M-05?

[defsec]

Agree, Zero address checks and two step procedures are marked as QA on other contests.

[IllIllI000]

zero address checks and two-step transfers have regularly been marked as QA following this post, including when withdraw-like functions are directly callable by users, and can be passed arbitrary addresses: https://discord.com/channels/810916927919620096/810936719003090974/937520228324229170 I agree that the incorrect implementation falls into a separate category, but #31 and #32 do not fit into it

[dmitriia]

Yes, two-step transfers are generally accepted to be QA non-critical and it is so in many other contests since then (it's pretty common issue).

code-423n4/org#1 (comment)

[dmitriia]

By the way, @dmvt if you decide otherwise, please upgrade (4) from #362 (and check for other QA reports occurrences, it can be many)

[dmvt]

Ok. I'm not 100% sure I agree, but I'll go with status quo on this one. I admit that I've gone back and forth in my own head about this issue and I'm not ready to make an strong enough argument that it would change the collective opinion of past C4 judges.

[thereksfour]

[Low-04] in #154 looks like a duplicate of #197 M-20

[dmvt]

I disagree that #154 is a dup of #197. #197 talks about the function being frozen due to a malicious token. #154 [4] talks about a gas limit being hit due to too many updates. I think #154 [4] is much less likely to happen than #197.

[thereksfour]

@dmvt After reading both reports carefully, I think they are describing the same problem. They both say that when the length of poolInfo is too large it causes DOS.
#197 doesn't say that malicious tokens cause DOS, it just says that massUpdatePools calls the updatePool function for the length of poolInfo, and If poolInfo.length is big enough, block gas limit may be hit.

[IllIllI000]

For #278 there was a recent discussion of the same issue which was found to be invalid that made sense to me. Reproducing here since the finding is not public yet, and @dmvt is not cert+:

@GalloDaSballo to clarify: the issue is not about the existence of lockers, which are indeed part of a composable system, but about the lack control <project name redacted> has over it. We've seen with Curve Finance and Frax for example that the locker whitelisting process is essential to ensure interests are aligned between the original DAO and the tokenized locker DAO

I think there is a space to debate the validity of whitelists, governance mechanisms as well as protocol design, however during this contest we are supposed to "prove" whether a system is safe or not.

I don't believe the finding has done that.

On another note, EIP-3074 will eventually break the guarantees of msg.sender != tx.origin meaning that it is in developers best interest to find a way to make their composable-contracts safe, rather than breaking composability.

I agree with your take that a whitelist has been historically used to defend DAOs for the better (thinking of curve gauges here), however in a way that in itself would be more of the result of the whitelisting process, more so than the requirement of it not being doable by a contract.

(If you check how Curve works, the 7 day voting is the security measure, the fact that there's a EOA guard is secondary as you could remove the guard and still retain "alignment of interest" within the 7 day voting), however ultimately I don't think this is the place to debate this as I don't think we can reach a actionable outcome without ultimately recommending the sponsor to either re-build the entire system or just use what they have.

In conclusion, while I think your opinion has merit, and I think it sparkled a healthy discussion, the finding doesn't illustrate a vulnerability in the system.

I'm fully aware of another instance of a judging rating this highly and believe there's room for disagreement on that as well, under different context and circumstances, given a different judge and sponsor, this finding could have been rated differently.

In order for me to consistently judge the contest though, I believe it is correct of me to maintain my original judgement of marking the finding invalid

[dmvt]

I appreciate you informing me of this but to the extent that the issue is the same (and because I don't have a link to the full issue I'm not 100% sure it is), I disagree with the other judge. In #278 any wallet can lock a token at will. I downgraded it to medium because I think this is not only dangerous but also very likely to occur. TBH I almost left this one as high severity, but decided not to because the DAO / multisig has other ways to mitigate the impact including blacklisting the wallet from governance, so I don't believe that it is likely this attack could lead to stolen funds.

[dmvt]

Here's my changelog based on this conversation:

Issue 301 - Medium -> Invalid
Issues 237, 221, 289 & 309 - Medium -> QA
Issues 85, 51, 102 & 103 - Medium -> QA
Issues 31, 244, 243, 361, 132 - Medium -> QA
Issue 32 - Medium -> QA

Super helpful add to the process, particularly for contests with a ton of issues.

[dmitriia]

Can you share the resulting sheet?

[dmvt]

The original sheet has been updated.

[CloudEllie]

Can you share the resulting sheet?

We don't really have a scalable, repeatable way to do this (i.e. share the finalized judging sheets) yet -- but stay tuned, we are building out features that will allow a lot more dynamic updating, and better visibility into the judging process.