Skip to content

wip: buy with stable/cusd#291

Merged
blueogin merged 54 commits intomasterfrom
feat/buygd-mento-glo
Mar 16, 2026
Merged

wip: buy with stable/cusd#291
blueogin merged 54 commits intomasterfrom
feat/buygd-mento-glo

Conversation

@sirpy
Copy link
Contributor

@sirpy sirpy commented Dec 22, 2025

Description

about: #290

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 3 issues, and left some high level feedback:

  • In BuyGDCloneV2.swap, the Bought event emitted in the cUSD/stable branch still uses celo as the token address, which will mislead consumers of the event and should be updated to emit the correct token (likely CUSD or stable).
  • In swapCusd, gasCosts are carved out from the contract’s CUSD balance but the refund is paid in stable, which will fail or behave unexpectedly when stable != CUSD; consider refunding from the same token the gas was reserved in or explicitly ensuring sufficient stable balance for the refund.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `BuyGDCloneV2.swap`, the `Bought` event emitted in the cUSD/stable branch still uses `celo` as the token address, which will mislead consumers of the event and should be updated to emit the correct token (likely `CUSD` or `stable`).
- In `swapCusd`, `gasCosts` are carved out from the contract’s `CUSD` balance but the refund is paid in `stable`, which will fail or behave unexpectedly when `stable != CUSD`; consider refunding from the same token the gas was reserved in or explicitly ensuring sufficient `stable` balance for the refund.

## Individual Comments

### Comment 1
<location> `contracts/utils/BuyGDClone.sol:93-101` </location>
<code_context>
 		uint256 gasCosts;
+		uint24[] memory fees = new uint24[](1);
+		fees[0] = 100;
 		if (refundGas != owner) {
-			(gasCosts, ) = oracle.quoteAllAvailablePoolsWithTimePeriod(
+			(gasCosts, ) = oracle.quoteSpecificFeeTiersWithTimePeriod(
</code_context>

<issue_to_address>
**issue (bug_risk):** Gas refund is taken from CUSD balance but paid out in `stable`, which can mis-account funds when `stable != CUSD`.

`swapCusd` subtracts `gasCosts` from the CUSD balance (`amountIn = ERC20(CUSD).balanceOf(address(this)) - gasCosts;`) but sends the refund in `stable` (`ERC20(stable).transfer(refundGas, gasCosts)`). When `stable != CUSD`, this reduces CUSD while spending an unreserved amount of `stable`, which can desync balances and underfund future swaps. Consider either refunding in CUSD or charging gas costs from `stable` consistently.
</issue_to_address>

### Comment 2
<location> `contracts/utils/BuyGDClone.sol:72` </location>
<code_context>
 			return bought;
 		}
-		balance = ERC20(cusd).balanceOf(address(this));
+		balance = ERC20(CUSD).balanceOf(address(this));
 		if (balance > 0) {
 			bought = swapCusd(_minAmount, refundGas);
</code_context>

<issue_to_address>
**issue (bug_risk):** `swap` only checks CUSD balance, even though the contract now supports a generic `stable` token.

The second branch still checks `ERC20(CUSD).balanceOf(address(this))` and calls `swapCusd`, so if `stable` is set to a different token and only that token is funded, this path never runs and those funds are never swapped. Either restrict the docstring to CUSD only, or update this branch to work off `stable` (and a corresponding `swapStable`) when `stable != CUSD`.
</issue_to_address>

### Comment 3
<location> `contracts/utils/BuyGDClone.sol:34` </location>
<code_context>

 	constructor(
 		ISwapRouter _router,
-		address _cusd,
+		address _stable,
 		address _gd,
</code_context>

<issue_to_address>
**nitpick:** Constructor parameter name `_cusd` in `DonateGDClone` is now misleading since it represents a generic stable token.

The parameter is still named `_cusd` in `DonateGDClone` while it now represents a generic stable token and is passed through to `BuyGDCloneV2`. Please rename it to `_stable` to align with `BuyGDCloneV2` and avoid confusion when deploying with non-cUSD stablecoins.
</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 22, 2025

wip: buy with stable/cusd

Generated at commit: b1e748dc7b84d382651edef2e91ea660626b908a

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
5
0
16
44
68
Dependencies Critical
High
Medium
Low
Note
Total
0
0
1
0
131
132

For more details view the full report in OpenZeppelin Code Inspector

@blueogin
Copy link
Collaborator

blueogin commented Jan 5, 2026

@sirpy
I’ve completed step 1 for BuyGDClone - the swap from cUSD to G$ works successfully via the cUSD -> GLO -> G$ path on Uniswap.

…functionality and tests for expected returns and error handling
…c route selection between Uniswap and Mento with corresponding tests for expected returns and event emissions
…tor BuyGDClone contract by removing unused swap functions and adding new TWAP calculation methods; enhance tests to compare Uniswap and Mento routes for better swap decision-making
…yGDClone test setup by removing redundant network reset
…k reset and commented out chain ID verification
…mplify initialization; update tests for new swap functionality
@blueogin
Copy link
Collaborator

@sirpy
Could you please review it

Copy link
Contributor Author

@sirpy sirpy left a comment

Choose a reason for hiding this comment

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

too many changes
this should be a simple change of adding a variable to swapCusd and swapCelo and renaming them to swapCusdWithPath and swapCelowithPath
and the new swapCusd and swapCelo should call the swapwithpath version with default path

@blueogin
Copy link
Collaborator

blueogin commented Mar 2, 2026

@sirpy
The feature could have been “add a path argument, rename to WithPath, and add thin swapCusd/swapCelo that call WithPath with the default path.”
Instead, the PR introduced internal functions for core logic and added getters. So the diff is large because of the new abstraction and refactor of existing internals and factory, not just the small public interface change

@blueogin
Copy link
Collaborator

blueogin commented Mar 11, 2026

@sirpy
Could you please take a look and review this pr?

@sirpy
Copy link
Contributor Author

sirpy commented Mar 12, 2026

added review

@blueogin
Copy link
Collaborator

@sirpy
Please review/check comments regarding default paths

@sirpy sirpy requested a review from blueogin March 16, 2026 15:39
@sirpy
Copy link
Contributor Author

sirpy commented Mar 16, 2026

@blueogin you can merge

@blueogin blueogin merged commit 259ca37 into master Mar 16, 2026
14 of 20 checks passed
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