Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
BuyGDCloneV2.swap, theBoughtevent emitted in the cUSD/stable branch still usesceloas the token address, which will mislead consumers of the event and should be updated to emit the correct token (likelyCUSDorstable). - In
swapCusd,gasCostsare carved out from the contract’sCUSDbalance but the refund is paid instable, which will fail or behave unexpectedly whenstable != CUSD; consider refunding from the same token the gas was reserved in or explicitly ensuring sufficientstablebalance 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
wip: buy with stable/cusd
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
… in BuyGDClone tests
…etwork mismatch in BuyGDClone tests
|
@sirpy |
…ap methods and error handling
…ds and tests for expected return and error handling
…ional scenarios for expected returns and error handling
…ew methods for expected returns and enhanced error handling in tests
…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
…ount for price fluctuations
…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
…nd updating fork setup with Celo RPC URL
…yGDClone test setup by removing redundant network reset
…d retrieve signer
…k reset and commented out chain ID verification
…ID verification and network reset logic
…ve block gas limit for improved test setup
….2 and adjust test setup in BuyGDClone tests
…verification and removing network reset after tests
…nsufficient whale balance in BuyGDClone tests
Co-authored-by: sirpy <hadar@gooddollar.org>
…d updating test cases for better clarity and efficiency
…or improved accuracy
…ions for accurate price comparison
…act and update tests for expected return validation
…ests for new initialization parameters
| * @param _owner The address of the owner of the contract. | ||
| */ | ||
| function initialize(address _owner) external initializer { | ||
| function initialize(address _owner, UniswapPath memory _cusdPath, UniswapPath memory _celoPath) |
There was a problem hiding this comment.
the swappath should be an input to the buy methods.
so it is the UI responsibility to set the correct path.
remove the changes you did, instead do:
- buygdclone instance should have hardcoded path defaults.
- add buyXWithPath methods for celo/cusd the regular buy methods should use the default path
…mplify initialization; update tests for new swap functionality
|
@sirpy |
sirpy
left a comment
There was a problem hiding this comment.
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
| * @return bought The amount of GD tokens received. | ||
| */ | ||
| function swapCusd( | ||
| function _swapCusdChooseRoute( |
There was a problem hiding this comment.
why is this required?
swapCusd should be converted to swapCusdWithPath adding a variable
new swapCusd should call swapCusdWithPath with default path
There was a problem hiding this comment.
_swapCusdChooseRoute is an internal function that handles the core logic for swapping cUSD.
Currently, swapCusd calls swapCusdWithPath using a default path, and swapCusdWithPath then calls _swapCusdChooseRoute to execute the core swap logic.
The 'ChooseRoute' in name means it selects the route between Mento and Uniswap.
| /** | ||
| * @notice Swaps cUSD for GD tokens via Uniswap using the given path. | ||
| */ | ||
| function _swapCUSDfromUniswap( |
There was a problem hiding this comment.
keep functions in order add new functions at the end so easier to review changes
…token swaps in BuyGDClone contract
… Uniswap path swaps
… for improved flexibility
|
@sirpy |
Description
about: #290