Skip to content

Conversation

@sirpy
Copy link
Contributor

@sirpy sirpy commented Dec 24, 2025

No description provided.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • In setBridgeFees the validation uses bridgeFees.fee <= 10000 instead of fees.fee <= 10000, so the require condition is checking the old value rather than the new input; this should be corrected to validate the new fees being set.
  • For consistency and clarity, consider validating the input struct fees.fee in the constructor as well (even though bridgeFees is immediately set from fees) so the require reads the same way in both constructor and setBridgeFees.
  • The private key selection logic in hardhat.config.ts relies on process.argv containing the substrings testnet or staging, which may be brittle across different invocation patterns; consider basing this on named environment variables or Hardhat network names instead.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `setBridgeFees` the validation uses `bridgeFees.fee <= 10000` instead of `fees.fee <= 10000`, so the require condition is checking the old value rather than the new input; this should be corrected to validate the new fees being set.
- For consistency and clarity, consider validating the input struct `fees.fee` in the constructor as well (even though `bridgeFees` is immediately set from `fees`) so the require reads the same way in both constructor and `setBridgeFees`.
- The private key selection logic in `hardhat.config.ts` relies on `process.argv` containing the substrings `testnet` or `staging`, which may be brittle across different invocation patterns; consider basing this on named environment variables or Hardhat network names instead.

## Individual Comments

### Comment 1
<location> `packages/bridge-contracts/contracts/messagePassingBridge/MessagePassingBridge.sol:268` </location>
<code_context>
         _onlyAvatar();
         if (amount == 0) amount = IERC20(token).balanceOf(address(this));
-        IERC20(token).transfer(msg.sender, amount);
+        require(IERC20(token).transfer(msg.sender, amount), 'withdraw failed');
     }

</code_context>

<issue_to_address>
**question (bug_risk):** Requiring the raw `transfer` return value may break compatibility with non-standard ERC-20 tokens.

The code previously ignored the `transfer` return value; now it assumes a `bool` is returned. Many popular or legacy ERC-20 tokens either don’t return a value or revert on failure, so this `require` will itself revert even when the transfer is actually successful under their behavior. If you need to support such tokens, use a SafeERC20-style wrapper that treats missing return values as success instead of requiring the raw `bool`.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@openzeppelin-code
Copy link

openzeppelin-code bot commented Dec 24, 2025

add: fixes + last upgrade addresses

Generated at commit: b2641546bddf25f0b132f7320370e99ebb65b82a

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
0
1
0
9
35
45
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants