Feature: oft adapter step2 - bridge limits and fees in OFTAdapter #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…gic and update dependencies
…ipient configuration
…contracts by removing bridge limits and minting logic, enhancing clarity and maintainability
…ncluding removal of outdated scripts and addition of remapping file
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GoodDollarOFTAdapter._credittherequestIdis derived fromblock.timestamp,_to,_srcEid, and_amount, which makes it impossible to deterministically pre-approve off-chain (you can’t know the timestamp ahead of time); consider either passing a requestId from the message payload or changing the approval model so it’s usable in practice. - The
BridgeFeesstruct includesminFeeandmaxFee, but_takeFeeand_creditcurrently only usefeeand ignore the min/max fields; either enforce these bounds in the fee calculation or remove the unused fields to avoid confusion. - The
approvedRequestsmapping inGoodDollarOFTAdapteris write-only and never cleared, so approved entries will accumulate indefinitely; consider adding a mechanism to delete or expire approvals after use to avoid unbounded storage growth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GoodDollarOFTAdapter._credit` the `requestId` is derived from `block.timestamp`, `_to`, `_srcEid`, and `_amount`, which makes it impossible to deterministically pre-approve off-chain (you can’t know the timestamp ahead of time); consider either passing a requestId from the message payload or changing the approval model so it’s usable in practice.
- The `BridgeFees` struct includes `minFee` and `maxFee`, but `_takeFee` and `_credit` currently only use `fee` and ignore the min/max fields; either enforce these bounds in the fee calculation or remove the unused fields to avoid confusion.
- The `approvedRequests` mapping in `GoodDollarOFTAdapter` is write-only and never cleared, so approved entries will accumulate indefinitely; consider adding a mechanism to delete or expire approvals after use to avoid unbounded storage growth.
## Individual Comments
### Comment 1
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:183-190` </location>
<code_context>
+ * @param amount The amount to calculate fee from
+ * @return fee The calculated fee amount
+ */
+ function _takeFee(uint256 amount) internal view returns (uint256 fee) {
+ fee = (amount * bridgeFees.fee) / 10000;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Bridge fee calculation ignores `minFee`/`maxFee` fields of `BridgeFees`.
`BridgeFees` exposes `minFee` and `maxFee`, but `_takeFee` only uses the BPS value and never enforces these bounds. This can mislead integrators into assuming on-chain caps that don’t exist.
If these fields should be enforced, consider clamping the computed fee, e.g.:
```solidity
uint256 raw = (amount * bridgeFees.fee) / 10000;
if (bridgeFees.minFee > 0 && raw < bridgeFees.minFee) fee = bridgeFees.minFee;
else if (bridgeFees.maxFee > 0 && raw > bridgeFees.maxFee) fee = bridgeFees.maxFee;
else fee = raw;
```
If they’re informational only, consider renaming or removing them to avoid implying enforcement in this function.
```suggestion
/**
* @notice Calculates the fee amount from the given amount
* @param amount The amount to calculate fee from
* @return fee The calculated fee amount
*/
function _takeFee(uint256 amount) internal view returns (uint256 fee) {
uint256 raw = (amount * bridgeFees.fee) / 10000;
uint256 minFee = bridgeFees.minFee;
uint256 maxFee = bridgeFees.maxFee;
if (minFee > 0 && raw < minFee) {
fee = minFee;
} else if (maxFee > 0 && raw > maxFee) {
fee = maxFee;
} else {
fee = raw;
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+183
to
+190
| /** | ||
| * @notice Calculates the fee amount from the given amount | ||
| * @param amount The amount to calculate fee from | ||
| * @return fee The calculated fee amount | ||
| */ | ||
| function _takeFee(uint256 amount) internal view returns (uint256 fee) { | ||
| fee = (amount * bridgeFees.fee) / 10000; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Bridge fee calculation ignores minFee/maxFee fields of BridgeFees.
BridgeFees exposes minFee and maxFee, but _takeFee only uses the BPS value and never enforces these bounds. This can mislead integrators into assuming on-chain caps that don’t exist.
If these fields should be enforced, consider clamping the computed fee, e.g.:
uint256 raw = (amount * bridgeFees.fee) / 10000;
if (bridgeFees.minFee > 0 && raw < bridgeFees.minFee) fee = bridgeFees.minFee;
else if (bridgeFees.maxFee > 0 && raw > bridgeFees.maxFee) fee = bridgeFees.maxFee;
else fee = raw;If they’re informational only, consider renaming or removing them to avoid implying enforcement in this function.
Suggested change
| /** | |
| * @notice Calculates the fee amount from the given amount | |
| * @param amount The amount to calculate fee from | |
| * @return fee The calculated fee amount | |
| */ | |
| function _takeFee(uint256 amount) internal view returns (uint256 fee) { | |
| fee = (amount * bridgeFees.fee) / 10000; | |
| } | |
| /** | |
| * @notice Calculates the fee amount from the given amount | |
| * @param amount The amount to calculate fee from | |
| * @return fee The calculated fee amount | |
| */ | |
| function _takeFee(uint256 amount) internal view returns (uint256 fee) { | |
| uint256 raw = (amount * bridgeFees.fee) / 10000; | |
| uint256 minFee = bridgeFees.minFee; | |
| uint256 maxFee = bridgeFees.maxFee; | |
| if (minFee > 0 && raw < minFee) { | |
| fee = minFee; | |
| } else if (maxFee > 0 && raw > maxFee) { | |
| fee = maxFee; | |
| } else { | |
| fee = raw; | |
| } | |
| } |
… by integrating IMessagePassingBridge structures, enhancing modularity and clarity
Feature: oft adapter step2 - bridge limits and fees in OFTAdapter
🚨 Report Summary
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR inclues smart contracts which implementes bridge limits and fees in GoodDollarOFTAdapter.sol
About #7
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: