Post-Judging QA

[thebrittfactor]

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 @alcueca.

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!

[alcueca]

Hey all, thanks for participating in this contest. I could see many of you made a huge effort, more so with such a complex codebase. I would like to give you some background on how I've judged it, so that you can more easily see if I made a mistake.

1. I've chosen to assume that the main users of MaiaDAO Ulysses Protocol won't be regular retail users, but other applications or UIs. In other words, no one is going to be mad enough to use etherscan with this. Therefore, it is expected of the user to check the documentation, ask the devs, and try their transactions in a fork first. If they make a mistake, or of their assumptions are wrong, it's their fault. In this light, all vulnerabilities that are due to such user errors are QA.
2. If the economic damage is gas lost somehow. It's a Medium.
3. With duplicates, I've favored submissions with PoCs because they are hard to create and they make it so that I don't have to ask so many reviews from the sponsors, which takes considerable time and effort from them.
4. On QA, reported suggestions that are included in the automated bot reports count against you. Best reports are those with a higher valid/invalid ratio, not the ones with most reports. I selected as best the one that checked the documentation for LayerZero and contained no errors.
5. On Gas, I selected as best two duplicates that found a case in which Ulysses overpays 150k gas, which is different in my view from gas getting stranded.

I think that's about it. This was a difficult contest. All the jumps between contracts and chains made it very difficult. Often the vital checks where many jumps away, and only a deep understanding would be enough to find out the flaws. I tried to be fair, but I'm sure I erred. Correct me, but be compassionate in doing so.

[QiuhaoLi]

@alcueca Just FYI. For 2, the stuck gas tokens are not used as gas, they are airdrop native token (that's why they are in the msg.value of the dst chain) (https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters, nativeForDst not gasAmount). The code comment use the gas token to show that they are the native token of the destination chain.

[alcueca]

From your link:

[Tendency001]

Thanks for the good work judging all of these issues ser, I left a comment in #528.
Also, thanks for helping us submit better reports.

[alcueca]

Sorry, after checking with the sponsors and being pointed at the right portion of the LayerZero code, it seems that the attack described is not possible. Details in the report.

[Tendency001]

Just left another comment ser, thank you

[alcueca]

Let's not push it.

[zhaojio]

Hello judge @alcueca , thank you for your hard work!
The issue about nonce is uint32, whether we should consider the case of DoS attacks, my report has explained the case of DoS attacks. #119

1. The cost of DoS attack is not high and not difficult, the attacker doesn't care if the cross-chain call succeeds, just increases the value of the nonce, my report is based on the relatively high gas consumption to calculate.
2. Redeploying bridge agen, without modifying the code, does not solve the problem of a DoS attack, because an attacker can launch the attack again.

If other reports just say that the uint32 value is too small without considering DoS attacks, is my report a duplicate? Also, should DoS attacks be H/M?

[alcueca]

I think that your calculations are off by a factor of 100 million. Details in the report.

[zhaojio]

Thanks for clarifying, I had a mistake.

[alexxander77]

Greetings @alcueca,

Firstly, thanks for the swift judging efforts on a vast & complex system.

I want to bring to your attention that I believe issue #679 is incorrectly been duplicated with the issues under #877.

Issue #877 concerns the use of signed messages and Virtual Accounts and in what circumstance they can be compromised, however, issue #679 mentions a different root cause that is concerned with unsigned messages (i.e no Virtual Account) and a particular wrongful assumption in the code when creating settlements that is contained in the execute() function in the MulticallRootRouter. In other words, if we follow the mitigation proposed by the sponsor that Contracts should not use Virtual Accounts - the vulnerability discussed in #679 still persist in that the code blindly assumes that the refundee on the Root is the same as & controlled by the recipient on the Branch. Please consider #679 a separate issue.

Lastly, I would like to point out that the "honeypot" issue #374 seems to require some degree of circumstance similar to the issues that are currently under #877. I believe the mentioned issues should be all classed as Medium or all classed as High, please consider re-evaluating.

[stalinMacias]

I'd like to add some comments about issue #679, Isn't the redemption of tokens actually executed on the source branch? The execution goes through root to update the state & balances to reflect the operation of users redeeming their deposit, and from root, the execution flows goes back to the source chain, where in the end, the settlementNonce is actually increased and the state is updated so that the user can proceed to actually redeem the tokens. So, even though refundee is the same recipient, that's not a problem since the settlement of tokens will happen in the source branch, where the user initiated the call.

As per the "honeypoot" issue #374 , I also think that should be classed as Medium, from the sponsor's words themselves "The circumstances do need to be unique, but the implications if this happened are high.", so, high impact, low prob === medium.

[stalinMacias]

@alcueca Please scratch what I said about the #374 , and look at the comment of another warden, that issue was reported by the bot, thus, should be OOS

[alexxander77]

Hello @stalinMacias, the only function I am aware that allows redemption of settlements is located in the Root -> RootBridgeAgent.sol ie redeemSettlement(), and the assets are moved through IPort.bridgeToRoot() and sent to the msg.sender on the Root (#679 describes how this can be problematic), there are no cross-chain calls.

[alcueca]

On #877 and #679, I agree on splitting them into separate issues. My original thinking is that all of them arose from the error on the part of the sponsor on assuming that each address is controlled by the same individuals in all chains. From there, the attacks can be many.

However, #679 (and its duplicate) point to specific lines where this assumption is a problem in quite a different way from #877 and its duplicates.

[alcueca]

On #374, I don't see the relation to #877 or #679. On #374, the attacker creates fake deposits for tokens that have not been deployed yet on a network, but that if they do they will have a predictable address.

On deciding severity, you need to think on what would happen if the vulnerability wouldn't have been reported. In the case of #374, MaiaDAO wouldn't have been monitoring for the fake deposits to happen, and attackers would have set them up with a high chance of being undetected. Once one of the tokens get deployed, attackers would wait for as long as they felt comfortable, and then they would redeem their fake deposits at a loss to the real depositors. The only precondition is that tokens with predictable addresses get deployed after the protocol goes live, which is likely.

On that line, all #374, #679 and #877 must be classified as High, because all of them would have caused user losses if undiscovered. #679 is more because of user error, but the sponsor didn't specify that account abstraction wallets were not to be used, and it is debatable whether they would have said it once the protocol is live but before account abstraction wallets are used (and potentially harmed).

[osmanozdemir1]

Hi @alcueca

Thanks for your efforts and judging this protocol quickly. Amazing work!

We left some comments under #685, #688, #689, #693 and #737.

Best wishes

[alcueca]

On #689, upgraded to grade-a QA

[osmanozdemir1]

1 is not wrong because of this line here. Funds are transferred to the caller, not the owner.

Caller gets the funds even though the deposit owner is different because of the payload is created with the caller address not the refundee address.

[alcueca]

On #693, despite the report being of excellent quality, I can't agree that this is an issue on how MaiaDAO uses LayerZero or cross-chain messaging in general. The report has an excellent description of cross-chain deposits and the issue of reorgs in cross-chain messaging, but lacks the proof that there is an issue to how MaiaDAO specifically implemented cross-chain messaging, and the mitigations proposed are vague enough to be almost non-existent, which makes me think that appropriate mitigations better than the default ones implemented by LayerZero might not exist.

[alcueca]

On #737, I can see clearly that the natspec is wrong (again). The first replenishReserves function takes assets from the caller to replenish reserves, the second one takes assets from one of the strategies in the port, with amounts determined by the port to be right. The second function can be though of as a rebalancing function, and it is a common pattern.

[osmanozdemir1]

Thanks for all of your efforts, reevaluating the issues, and patiently answering all questions. I appreciate it.

On #737, I totally get the idea to increase the reserves, but it won't work. Why would a strategy contract takes tokens from the port but not use these tokens? The idea of the strategy contracts is taking tokens from the port and investing them. Tokens will not just sit in the strategy contract. Letting everyone force strategy contracts to pay their debt to increase reserves will not work. But, I get it. It can't be considered as a bug.
Thanks for your time.

[DevHals]

Hi @alcueca ,,
I have left a comment on issue #433
also regarding issue #374 : this issue is reported by the winning bot (OOS)L-13,

Thanks,

[stalinMacias]

I knew it was almost impossible the bots didn't catch that issue, thanks for flagging this out sir, as per the rules, #374 should be OOS, let's wait for the final judge's veredict

[peakbolt]

In my opinion, 374 is valid because it has escalated the Low issue to a higher severity issue with an attack path. Hats off to the wardens for catching this.

[alcueca]

In that sense, I agree with @peakbolt. Without explicit impact, as in the bot report, the finding is a Low. With a specific impact, it becomes an HM.

Generally, in those cases, I upgrade the report that included a vulnerability as a lower severity, but apply a 50% or 25% reward for missing the larger impact.

[alcueca]

The sponsor replied on #433 with a detailed flow. I hope that's enough.

[bin2chen66]

Hi @alcueca , thank you for your hard work!
I have left a comment on the issue #610
Thanks

[alcueca]

You were right, and #610 is a valid medium. #610 is now best, and #822 and #860 are duplicates.

[securitygrid]

Hi @alcueca, thank you for your hard work!
I have left a comment on issue #520 and #518.

[securitygrid]

I would like to describe here my understanding of sending messages in this protocol. I roughly divide the messages into 2 categories:

1. branch->root, this type of message means that it is sent from branch to root. This does not require Airdrop to give arbitrary gas to the rootAgent at all. If the user provides gas, it is the user's own fault. I think the protocol will provide corresponding UI to prevent users from paying unnecessary gas.
2. branch->root->branch, this type of message requires sending a message back to branch on root. Airdrop gives gas to RootBridgeAgent for this purpose: to enable root to send messages internally, so msg.value will be passed with the inner call. However, if msg.value is not passed in any step, it will cause endpoint.send{value:0} to fail internally (endpoint will internally check whether there is enough value). What problems will this ultimately cause? The msg.value that has not been passed still remains in the RootBridgeAgent (due to the implementation of address(this).excessivelySafeCall) and will not be stuck in any contract in the intermediate process. In other words, the native token airdropped to RootBridgeAgent will only be stuck in RootBridgeAgent because the msg.value passed to Endpoint.send is 0, causing an internal revert.

If you think the above description is correct, then some issues need to be reconsidered. If it is wrong, please ignore it.
Thanks.

[alcueca]

#520 is not a duplicate of #896, and is valid. Apologies for the misjudgement.

[securitygrid]

Hi, I left some messages on #518 a few days ago. Please recheck it.

[alcueca]

I was just working on that one. I marked #518 as best, and #464 as duplicate. The latter seems to have found the impact, but not the root cause.

[Topmark1]

Hi @alcueca, thank you for your great effort in this protocol!
I have left a comment on issue #218

[alcueca]

The sponsor was kind enough to show why the array lengths are the same. An alternative path to arrive to that internal function is not shown, so the report is invalid.

[stalinMacias]

Hello @alcueca , first of all, thanks for the incredible good work on judging this contest, the quality of the judging is notorious, great work.

I'd like to ask about your comment made on the issue #439 , which are the reports getting full rewards and which ones are getting 50%?
Also, I submitted an issue about this, and I submitted it as a high issue because the existing implementation that was audited would completely break the whole purpose of validating if the caller is indeed the expected contract, so, I still think this finding deserves a high severity, please, do take a look at my report about this same issue #86

I added a comment on #332 , just to keep it in the loop in case its main issue's severity is updated.

I added a comment on #287 , I think that there was a misunderstanding about what I'm reporting, I already tagged the sponsor, but If you could help me bring this issue back to the sponsor's attention, I'd really appreciate it.

Thanks for taking the time to check my comments

[alcueca]

On #86, and other duplicates of #348, it is a Medium because no assets would be lost. The protocol would not be viable after deployment and would need to be fixed and redeployed.

[alcueca]

On #332, see what I said below.

[alcueca]

On #287, I've asked for further feedback from the sponsor

[alcueca]

#881 stays as Medium, I don't see it obvious that an ecosystem token should be added to Ulysses in the sae transaction as it is deployed, unless I don't understand what an ecosystem token is.

[alcueca]

On #287, it's invalid. I think that by now it is clear that the protocol is working with regards to the feature reported.

[3docSec]

Hi, below a few comments:

• #611 seems invalid: the msg.sender == getBranchBridgeAgent[localChainId] will always be true when the sender is ArbitrumBranchBridgeAgent(unless the Arbitrum branch is incorrectly set up by the admin) therefore reverting and preventing fund locks; it is quite likely that this if was explicitly made to avoid that very finding. At the very best (and with some stretch IMO), this could be a dup of #464.

• #374 is L-13, so should be invalid

• #351 was grouped with #877, but even though the incorrect assumption is the same, the impact is different and the mitigation offered by #877 would not work for rescuing locked native tokens. My understanding is that findings are grouped together with the primary finding mentioning the different instances; #351 already enumerates all instances of the same issue, so I would ask to kindly review if it is fair to consider it as a group finding per se.

Thanks

[alcueca]

Marked #611 as invalid, thank you.

[alcueca]

On #374, I've asked for feedback from the wider judging community, it could set a precedent and I want to be careful.

[alcueca]

On #351, I'm not that sure what it is that you are asking. If it helps, multiple findings for an underlying bug are usually grouped. As always, there is some judge discretion. When they are grouped, the one mentioning all the instances is usually chosen as best.

In this case, I chose as best one that doesn't mention all the instances, for reasons explained in the finding. The sponsor is then directed to check the duplicates so that he sees all the instances. Mitigations are suggestions for a fix, and in a group of duplicates they are inevitably biased to the report they appear in. It is the job of the sponsor to decide which mitigation is best after reading all the findings, if any of them is suitable and if the sponsor decides to fix the bug.

[alcueca]

On #374, I've decided to judge it as out of scope. It is a difficult case, because it provides significant value to the sponsor, but accepting it as valid would undermine future contests by casting uncertainty around the bot race rules.

[peakbolt]

Hi @alcueca,

Thank you for judging this contest expeditiously.

I would like to request your time to review #422, #611 and #881. Have left comments in them.

Please also consider selecting #359 instead of #813 as the best report, because it explains the issue better with code and inline comments, and it also has instructions under POC that describes how to verify the issue quickly.

[Tendency001]

Since sending this gas along the call isn't necessary, imo it falls down to a user error.
Just like #719, the will need to be a fix, even sponsor confirmed it before I submitted, but I admit it will have to be a user error for the said loss.

[alcueca]

On #422, I agree that it is not a duplicate of #430, but given the limited impact, stays as QA.

[alcueca]

On #359, I've reduced the severity to QA (a), as it is clearly user error.

[udsene]

Hello @alcueca,

In the issue #279 there is reference to this issue where the sponsor has made the following comments:

It is not intended, thanks for noticing it! Please submit it! It does not affect our current routers, but it can impact other implementations so it breaks possible features. It should also have: {value: msg.value} as other calls in RootBridgeAgentExecutor.

In the above statement sponsor mentions that the msg.value should be passed along the executeSystemRequest function call.

In the issue #359 also the sponsor states the following:

Missing passing msg.value in IRouter(_router).executeResponse(_payload[PARAMS_TKN_START:], _srcChainId);. This leads to msg.value being stuck in the executor contract.

Can I please know why the sponsor thinks the msg.value should be passed along the executeSystemRequest function call. This is to get a better understanding on this vulnerability.

Thank You

[alcueca]

On #279, the sponsor comments are consistent with a QA severity as this contest has been judged (no current impact).
On #359, the sponsor notes that the gas remains stuck in the executor contract because the executor contract doesn't pass it on. That is true. However, the gas making it to that function in the first place can only happen through user error, and therefore this is QA severity.

[rvierdiiev]

hello @alcueca
i have left comments in #190, #179
thanks

[alcueca]

#190 could have used a bit more love to get full rewards. On #179, I see it as an exceptionally good gas report.

[rvierdiiev]

could have used a bit more love to get full rewards

good words)

i understand, thank you

[lokithe5th]

Hi @alcueca,

Thank you for your judging efforts, it is appreciated!

I have left a comment on issue #399

Thank you

[alcueca]

Replied, with colour.

[lokithe5th]

Thank you for taking the time to have another look, I appreciate it.

[mcgrathcoutinho]

Hi @alcueca, thank you for your hard work and quick judging on this complex codebase.

I have left my comments on #458 and #564.

I appreciate you taking the time to read this.

[alcueca]

#458 is now selected as best, congrats.

[alcueca]

I actually agree with the comment on #564, and can't really glean much from #190. If that report would have been presented on its own, without the context provided by #564, it would probably have been discarded as insufficient proof.

[thangtranth]

Hi @alcueca,

Thanks for your judging. I left a comment on #761. Can you please help me to review? Thank you.

[alcueca]

Accepted as QA grade-a, due to the hypothetical harm scenario.

[viktorcortess]

Hello @alcueca, I left a comment regarding severity on 649, and my duplicates on 279, 377, 548

Thanks for your attention.

[alcueca]

On #649, MKR only has bytes32 names on mainnet, and future tokens are unlikely to not have string names. Valid QA, but that's it.

[alcueca]

Made #279 a duplicate of #359 at 25%, because I'm feeling generous. You just found something unusual, with no impact described. Could as well mark it as insufficient quality.

[alcueca]

On #377, as before, the identification of a weak point didn't include an actual exploit of note. This time, I don't see such a close resemblance, and I'm not upgrading it.

[khalidfsh]

Hi @alcueca
I appreciate your time and the incredible awesome work on judging,
Could you revisit #771, left a comment waiting for your opinion.

[alcueca]

#771 is complete nonsense, did you get chatGPT to write it?

[khalidfsh]

Yes, ChatGPT helped me to explain it and unfortunately we failed! But the idea is mine.
Thank you for your helpful comment.

[alcueca]

I have nothing against using chatGPT to act as an editor if your own writing in English is not great, but you need to control it so that it doesn't say garbage. Not only the idea needs to be yours, but also the identification of the impact, and the proof. Use chatGPT at the end so that what you say makes grammatical sense and can be understood, but don't let it make stuff up.

[emmydev9]

@alcueca Thanks for judging this.
I left a comment on #264

[alcueca]

You are right, restored the full credit

[JeffCX]

left comments on #374

[alcueca]

Marked it as out of scope now

[JeffCX]

Thanks!

[khalidfsh]

left a comment on #877

[alcueca]

Stays as High

[stalinMacias]

Left comment on #679

[alcueca]

I think that this doesn't need a review now, right?

[stalinMacias]

That's right, good catch by the warden, I had my doubts about the finding, but ultimately looks a valid finding

[alcueca]

Hey all, I might not be able to respond to every comment until sometime next week. Each one of your appeals takes me a considerable amount of time and effort, and I have little availability until next Monday.

[alcueca]

Ok, so I'm back at dealing with your questions, all day today. Please allow me to add some more clarity on how I'm going to judge this, so that there is some consistency:

To be a High or a Medium in this contest:

• Users (or attackers) hurt other users, or
• Users hurt themselves by doing something they were expected to do by the sponsor.

If users hurt themselves by doing something that no one told them to do, I'm going to classify that as a QA.

There are plenty of Highs and Mediums already, so it is not a very high bar. Not everything can be a Medium or High, there needs to be some grading between reports.

[peritoflores]

Hi judge @alcueca you marked my submission as dup of the so common payableCall lacks access control. However, I am showing that there is a vulnerability inside the function itself to drain ETH. (You need to exploit a overflow in an unchecked block and a trick in a boolean variable).. I am sure you were pretty busy that you didn't take a look but the bug is already there. Can you take a second look? I left a comment in #687
The sponsor probably didnt see as it was presorted quickly

[alcueca]

The Post-Judging QA process is closed, sorry.