Post-Judging QA

[PaperParachute]

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 @kirk-baird .

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!

[rodiontr]

Hi @kirk-baird ! Thank you for you judging. Would really appreciate it if you look at issue #280 again. And if it's actually low severity maybe it would also affect making the QA report of grade b.

[kirk-baird]

Hi @rodiontr, it's a interesting edge case for #280 whether to allow zero share transfers or not. This is an issue similar to ERC20 transfer of zero amount. While it makes sense to reject zero transfers to prevent spam, this has caused a lot of issues for external projects often resulting in DoS vectors if 3rd party contracts don't handle the zero case specifically.

However, the point you raise is valid and I'll accept the issue as non-critical and add some comments on the issue for the sponsor. I don't believe the QA is sufficient to qualify for grade-b. Note there are only two issues (this would make 3) in your report and L-02 is already found by the bot here.

To to summarize it's more common to argue that reverting on zero share transfer will cause more issues than the benefit of preventing spam.

[BenRai1]

Hi @kirk-baird, thanks for finishing the judging so fast. Could you have a look at #265 , #118 and #258. Thanks in advance :)

[kirk-baird]

Hi @BenRai1, added some comments to the issues and waiting to hear back from the sponsor to decide how to rate #118

[c3phas]

Hey @kirk-baird ,not sure but are this two reports the same person #465 and #504 , also left some comments on them regarding some invalid and OOS issues

[kirk-baird]

Hi @c3phas thanks for looking through these, they are identical except ordering of issues, I'll look more into this.

[c3phas]

Hey @kirk-baird left some comments on #390 , and #518 ,#132 most of the issues are either invalid or OOS and should be grade b at best if not grade c. If wardens keep reporting issues already submitted by the bot it would not be fair for others

[kirk-baird]

@c3phas thanks for sorting through these and apologies for missing so many of the bot findings. I agree that wardens should not be rewarded for submitting out of scope issues. Will downgrade to b or c.

[alymurtazamemon]

Hello @kirk-baird,

I have left a comment on the issue #402

Thanks

[stalinMacias]

Hi @kirk-baird , incredible work judging this contest in a short period of time.

I believe my finding #100 is actually a duplicate of #136 and not a duplicate of #313

Could you take a second look? In the description, I'm talking about how a user can prevent an admin burns their tokens, and the reasons I mentioned are exactly the same reasons mentioned in #136 . While I made more emphasis on how a user could prevent their tokens from being burnt, #136 explains if a user is blocklisted then the admin can't burn their tokens. Which at the end of the day is the same underlying reason I explained in my report

[stalinMacias]

@kirk-baird
I added some comments to my reports #101 & #102 . Please take a look at them and let me know what do you think about it

[kirk-baird]

Agreed with you on #100 and chagned the duplication, thanks for pointing this out!
Also agreed about the duplication on #101 but consider this a Low.
Provided some comments on #102

[stalinMacias]

@kirk-baird I added a comment on #162 which is a duplicate of #391
Please let me know if you disagree with my perspective about that

[JeffCX]

left comments on #313

@kirk-baird

Thanks for judging the contest!

[0xRizwan]

Hi @kirk-baird,

Thanks for the quick judging.

Please check #331 and #508

Thank you!

[kirk-baird]

Thanks, I've replied on the issues.

[0xnirlin]

Hey @kirk-baird
Thanks for the swift judging. Left comments on #419 #495 and #516. Will really appreciate the second look. Thank you.

[kirk-baird]

Replied to the comments on each issue.

[romeroadrian]

Hi @kirk-baird , I have a few comments related to some of my submissions, would appreciate your feedback from these points.

• I think Approver could accidentally DoS their approval by executing the bridged message #441 was mistankenly merged with Approver accounting for transactions in Destination Bridge is off by one, could be one less approver is required. #32. In Approver could accidentally DoS their approval by executing the bridged message #441 I describe the case that a whitelisted approved executes the message (eg axelar service failed to call the execute message and needs to be done manually, possible since its permissionless), but this action then adds the account to the already approved list, which makes it impossible for the approve to do the "real" approval.

• It would be great to have the sponsor input about Bridged messages can still be approved and executed even if contract is paused #443, I guess that once the tokens are bridged you still want to have that pause preventing the minting. For example, a vulnerability allows multiple tokens to be bridged, contracts are paused, and an approver accidentally (or in connivance) approves those messages and mints the tokens.

• Not sure why Chain support cannot be removed or cleared in bridge contracts #444 was closed as invalid. Given the way that those mappings are filled (by converting an address to a string in one contract, and by hashing the string in the other), it would be impossible to clear those mappings as raymond states here Removing an already added src/dest contract address is not possible #467 (comment) . I think the issue is pretty well described in Chain support cannot be removed or cleared in bridge contracts #444 and should be valid.

• Even if Multicall feature in SourceBridge may be abused to send arbitrary messages to the Axelar service #445 falls in a centralization risk topic, I think it should be revisited. Having arbitrary calls in a contract that intefaces with the bridge is major footgun.

• I believe issue Insufficient precision while deriving price in oracle may lead to innaccurate calculations #449 was incorrectly merged with Wrong calculation of elapsed days #492. In Insufficient precision while deriving price in oracle may lead to innaccurate calculations #449 I describe the potential "leftovers" in the interest calculation that could be caused by precision issue, it's not about a +-1 offset.

Thank you

[kirk-baird]

Thanks for the feedback @romeroadrian

• I've left some comments on Approver could accidentally DoS their approval by executing the bridged message #441 as to why it's invalid
• Bridged messages can still be approved and executed even if contract is paused #443 is a clear QA in my opinion. New transactions cannot be sent into the bridge so it's only inflight transactions. It is a choice of the approvers and protocol whether to continue minting in-flight transactions. If they wish to stop minting the approvers may cease approvals.
• You're right the comment in Removing an already added src/dest contract address is not possible #467 is incorrect, I'll re-instate this issue and select a best.
• For Multicall feature in SourceBridge may be abused to send arbitrary messages to the Axelar service #445 this is a centralisation risk I see covered by the README which is generally out of scope. It is an interesting case and so I'll mark this as QA
• Agreed Insufficient precision while deriving price in oracle may lead to innaccurate calculations #449 is not a duplicate of Wrong calculation of elapsed days #492 . I'll adjust this, though severity is QA.

[DevHals]

Hi @kirk-baird,
I got 3 of my issues downgraded to QA with grade-c (as I didn't submit a QA-report for this contest),
aren't these issues eligible to get a higher grade?
issues : #84 , #85 , #88

[JeffCX]

left more comments on issue #313

[Jnrlouis]

Hello @kirk-baird, Excellent work judging this contest.

Please I need clarity on this #24

[khalidfsh]

Hello @kirk-baird,
Hope you're doing well! I wanted to revisit the event emission issue, offering a bit more context on its importance here #367

[kirk-baird]

Thanks for sharing @khalidfsh I've left some comments on the PR.

[0xnirlin]

Hey @kirk-baird
#437 is wrongly dup, it is invalid, bridges are not deployed deterministically.

[romeroadrian]

Author here. Thanks for flagging Ahmad!

I think it can be moved into a separate issue. As I've described in the report, it doesn't necessarily need to be deployed deterministically. A deployer account executing a script will have the same results.

[0xnirlin]

IMO this is QA at its best for following reasons:

1. I asked sponser the same question and they said there are no plans to deploy the contracts deterministically, they will take care of addresses themselves.

2. If the contract can be deployed to same address using the deterministic approach and accidentally in nondeterministic way than there is no obvious route than checking for it manually. As both lead to same ending.

3. If deployed to same address, its an admin/deployer mistake, first while deploying and than while setting two same addresses in the addChainSupport function.

4. Its a design choice, where each contract on different chains will be deployed to different addresses.

Yes this can be a good QA to add the mitigation you recommended.

[kirk-baird]

Thanks for flagging, I agree it's not a duplicate.
This is a good issue and does boarder between Medium and Low severity.

To have the same addresses on multiple chains the contracts need to be deployed from the same address with the same account nonce and contract bytecode. This could happen when a new deployer address (since the nonce is zero) is used across chains and contracts deployed from this address in the same order (with script or manually).

However, most developers deliberately avoid using one deployer address on multiple chains for this reason so I'm considering marking this QA.