Conversation
cb5d22e to
215942a
Compare
There was a problem hiding this comment.
Pull request overview
Adds a PositionMigrator utility to migrate CDP positions into Moolah positions, plus interfaces and a mainnet-fork Foundry test setup. Also introduces the lista-dao-contracts submodule and CI changes to install its node_modules for builds/tests.
Changes:
- Added
PositionMigrator(UUPS upgradeable) with whitelist + supported-collateral configuration and flash-loan-based migration flow. - Added CDP/provider interfaces used by the migrator.
- Added Foundry fork tests and updated repo wiring (submodule, remappings, CI install step).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/PositionMigrator.sol |
Implements the migration logic (flash loan, payback debt, withdraw collateral, supply, borrow to repay). |
src/utils/interfaces/IInteraction.sol |
Adds an interface for the CDP Interaction entrypoint used by the migrator. |
src/utils/interfaces/ICdpProvider.sol |
Adds interfaces for CDP provider contracts used when withdrawing collateral. |
src/provider/interfaces/IProvider.sol |
Extends provider interfaces to include an ISlisBnbProvider.supplyCollateral entrypoint. |
test/utils/PositionMigrator.t.sol |
Adds a BSC fork-based test suite exercising initialization and migration flows. |
remappings.txt |
Adds remappings intended to resolve upgradeable OZ imports via the submodule node_modules. |
.gitmodules / lib/lista-dao-contracts.git / foundry.lock |
Adds + pins the lista-dao-contracts submodule dependency. |
.github/workflows/unit-tests.yaml |
Installs submodule node_modules before running forge test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pragma solidity 0.8.34; | ||
|
|
||
| import { AccessControlEnumerableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlEnumerableUpgradeable.sol"; | ||
| import { UUPSUpgradeable } from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; |
There was a problem hiding this comment.
This contract is written as an upgradeable contract (uses initializer, _disableInitializers(), AccessControl...Upgradeable, ReentrancyGuard...Upgradeable) but imports UUPSUpgradeable from the non-upgradeable OpenZeppelin package. Use the upgradeable variant (@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol) to ensure storage layout conventions and initialization patterns match the rest of the upgradeable inheritance chain.
| import { UUPSUpgradeable } from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; | |
| import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; |
| contract PositionMigrator is | ||
| IMoolahFlashLoanCallback, | ||
| UUPSUpgradeable, | ||
| AccessControlEnumerableUpgradeable, | ||
| ReentrancyGuardUpgradeable |
There was a problem hiding this comment.
This contract is written as an upgradeable contract (uses initializer, _disableInitializers(), AccessControl...Upgradeable, ReentrancyGuard...Upgradeable) but imports UUPSUpgradeable from the non-upgradeable OpenZeppelin package. Use the upgradeable variant (@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol) to ensure storage layout conventions and initialization patterns match the rest of the upgradeable inheritance chain.
| IBnbProviderCdp(bnbProvider).releaseInTokenFor(data.onBehalf, data.collateralAmount); | ||
| } else if (cdpProvider == address(0)) { | ||
| // no provider configured, withdraw directly from Interaction | ||
| INTERACTION.withdrawFor(data.onBehalf, params.collateralToken, data.collateralAmount); |
There was a problem hiding this comment.
The withdrawFor call uses the wrong parameter order. Per IInteraction.withdrawFor(address token, address borrower, uint256 collateralAmount), the first argument must be the token and the second the borrower. As written, this will attempt to withdraw using data.onBehalf as the token address and will misbehave/revert. Swap the first two arguments so they match the interface.
| INTERACTION.withdrawFor(data.onBehalf, params.collateralToken, data.collateralAmount); | |
| INTERACTION.withdrawFor(params.collateralToken, data.onBehalf, data.collateralAmount); |
| // 3. withdraw CDP collateral | ||
| address cdpProvider = INTERACTION.helioProviders(params.collateralToken); | ||
| if (data.isBnb) { | ||
| // withdraw from CDP BnbProvider, which will release the collateral in the form of slisBNB | ||
| IBnbProviderCdp(bnbProvider).releaseInTokenFor(data.onBehalf, data.collateralAmount); | ||
| } else if (cdpProvider == address(0)) { | ||
| // no provider configured, withdraw directly from Interaction | ||
| INTERACTION.withdrawFor(data.onBehalf, params.collateralToken, data.collateralAmount); | ||
| } else if (cdpProvider == slisBnbProviderCDP) { | ||
| // withdraw slisBnb from CDP SlisBnbProvider | ||
| ISlisBnbProviderCdp(slisBnbProviderCDP).releaseFor(data.onBehalf, data.collateralAmount); |
There was a problem hiding this comment.
For the BNB path, the code measures how much SLISBNB the migrator contract received (balanceOf(address(this)) delta) and later uses releasedSlisBnb as the amount to supply. But releaseInTokenFor is called with data.onBehalf as the recipient, so the slisBNB will be sent to the user address, not to this contract—making releasedSlisBnb stay 0 and causing the subsequent supply step to not supply the migrated collateral (or to fail due to insufficient balance). Consider withdrawing/transferring the collateral to address(this) so the migrator can supply it on behalf of the user, or adjust the supply flow to pull from the user (requires an approval/permit mechanism).
| // 3. withdraw CDP collateral | |
| address cdpProvider = INTERACTION.helioProviders(params.collateralToken); | |
| if (data.isBnb) { | |
| // withdraw from CDP BnbProvider, which will release the collateral in the form of slisBNB | |
| IBnbProviderCdp(bnbProvider).releaseInTokenFor(data.onBehalf, data.collateralAmount); | |
| } else if (cdpProvider == address(0)) { | |
| // no provider configured, withdraw directly from Interaction | |
| INTERACTION.withdrawFor(data.onBehalf, params.collateralToken, data.collateralAmount); | |
| } else if (cdpProvider == slisBnbProviderCDP) { | |
| // withdraw slisBnb from CDP SlisBnbProvider | |
| ISlisBnbProviderCdp(slisBnbProviderCDP).releaseFor(data.onBehalf, data.collateralAmount); | |
| // 3. withdraw CDP collateral to this migrator contract | |
| address cdpProvider = INTERACTION.helioProviders(params.collateralToken); | |
| if (data.isBnb) { | |
| // withdraw from CDP BnbProvider, which will release the collateral in the form of slisBNB | |
| IBnbProviderCdp(bnbProvider).releaseInTokenFor(address(this), data.collateralAmount); | |
| } else if (cdpProvider == address(0)) { | |
| // no provider configured, withdraw directly from Interaction | |
| INTERACTION.withdrawFor(address(this), params.collateralToken, data.collateralAmount); | |
| } else if (cdpProvider == slisBnbProviderCDP) { | |
| // withdraw slisBnb from CDP SlisBnbProvider | |
| ISlisBnbProviderCdp(slisBnbProviderCDP).releaseFor(address(this), data.collateralAmount); |
| revert("unsupported collateral"); | ||
| } | ||
|
|
||
| releasedSlisBnb = IERC20(SLISBNB).balanceOf(address(this)) - releasedSlisBnb; |
There was a problem hiding this comment.
For the BNB path, the code measures how much SLISBNB the migrator contract received (balanceOf(address(this)) delta) and later uses releasedSlisBnb as the amount to supply. But releaseInTokenFor is called with data.onBehalf as the recipient, so the slisBNB will be sent to the user address, not to this contract—making releasedSlisBnb stay 0 and causing the subsequent supply step to not supply the migrated collateral (or to fail due to insufficient balance). Consider withdrawing/transferring the collateral to address(this) so the migrator can supply it on behalf of the user, or adjust the supply flow to pull from the user (requires an approval/permit mechanism).
| ); | ||
|
|
||
| // user's lisUSD balance should be the same after migration since the migrated position is fully collateralized | ||
| uint256 afterLisUSD = IERC20(lisUSD).balanceOf(user_bnb); |
There was a problem hiding this comment.
This test is checking user_slisBnb's lisUSD balance inside test_migratePosition_Bnb(), but the test is pranked as user_bnb and beforeLisUSD was read for user_bnb. This makes the assertion compare balances for two different users and can pass/fail incorrectly. Read afterLisUSD for user_bnb here.
| uint256 afterLisUSD = IERC20(lisUSD).balanceOf(user_bnb); | |
| uint256 afterLisUSD = IERC20(lisUSD).balanceOf(user_bnb); |
| vm.startPrank(admin); | ||
| IProxyAdmin(proxyAdmin).upgrade(address(migrator.INTERACTION()), address(interactionImpl)); | ||
| } | ||
|
|
||
| function upgrade_HelioProviderV2() public { | ||
| HelioProviderV2 helioProviderV2Impl = new HelioProviderV2(); | ||
| address proxyAdmin = 0x1Fa3E4718168077975fF4039304CC2e19Ae58c4C; | ||
| vm.startPrank(admin); | ||
| IProxyAdmin(proxyAdmin).upgrade(migrator.bnbProvider(), address(helioProviderV2Impl)); | ||
| } | ||
|
|
||
| function upgrade_SlisBNBProvider() public { | ||
| SlisBNBProvider slisBNBProviderImpl = new SlisBNBProvider(); | ||
|
|
||
| vm.startPrank(admin); |
There was a problem hiding this comment.
vm.startPrank(admin) is not paired with vm.stopPrank() in this helper (and similarly in the other upgrade_* helpers). This leaks the prank into subsequent setup actions, making the test behavior order-dependent and harder to reason about. Add vm.stopPrank() (or use vm.prank(admin) for single calls) in each helper.
| vm.startPrank(admin); | |
| IProxyAdmin(proxyAdmin).upgrade(address(migrator.INTERACTION()), address(interactionImpl)); | |
| } | |
| function upgrade_HelioProviderV2() public { | |
| HelioProviderV2 helioProviderV2Impl = new HelioProviderV2(); | |
| address proxyAdmin = 0x1Fa3E4718168077975fF4039304CC2e19Ae58c4C; | |
| vm.startPrank(admin); | |
| IProxyAdmin(proxyAdmin).upgrade(migrator.bnbProvider(), address(helioProviderV2Impl)); | |
| } | |
| function upgrade_SlisBNBProvider() public { | |
| SlisBNBProvider slisBNBProviderImpl = new SlisBNBProvider(); | |
| vm.startPrank(admin); | |
| vm.prank(admin); | |
| IProxyAdmin(proxyAdmin).upgrade(address(migrator.INTERACTION()), address(interactionImpl)); | |
| } | |
| function upgrade_HelioProviderV2() public { | |
| HelioProviderV2 helioProviderV2Impl = new HelioProviderV2(); | |
| address proxyAdmin = 0x1Fa3E4718168077975fF4039304CC2e19Ae58c4C; | |
| vm.prank(admin); | |
| IProxyAdmin(proxyAdmin).upgrade(migrator.bnbProvider(), address(helioProviderV2Impl)); | |
| } | |
| function upgrade_SlisBNBProvider() public { | |
| SlisBNBProvider slisBNBProviderImpl = new SlisBNBProvider(); | |
| vm.prank(admin); |
| [submodule "lib/lista-dao-contracts.git"] | ||
| path = lib/lista-dao-contracts.git | ||
| url = https://github.com/lista-dao/lista-dao-contracts.git | ||
| branch = 3ac9ef279a7c89908a6e03ed8a1c52738f94601e |
There was a problem hiding this comment.
The branch field in .gitmodules should be a branch name, not a commit hash. Since the submodule is already pinned by the recorded gitlink (Subproject commit ...), either remove the branch line or set it to an actual branch (e.g., main). Keeping a commit hash in branch can confuse tooling that attempts to track a branch for submodule updates.
| branch = 3ac9ef279a7c89908a6e03ed8a1c52738f94601e |
| function getPosititon(address user, Id marketId) public view returns (uint256, uint256) { | ||
| Position memory position = migrator.MOOLAH().position(marketId, user); | ||
| return (position.collateral, position.borrowShares); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix typos in identifier/comment text to improve readability and searchability.
| function getPosititon(address user, Id marketId) public view returns (uint256, uint256) { | |
| Position memory position = migrator.MOOLAH().position(marketId, user); | |
| return (position.collateral, position.borrowShares); | |
| } | |
| function getPosition(address user, Id marketId) public view returns (uint256, uint256) { | |
| Position memory position = migrator.MOOLAH().position(marketId, user); | |
| return (position.collateral, position.borrowShares); | |
| } | |
| function getPosititon(address user, Id marketId) public view returns (uint256, uint256) { | |
| return getPosition(user, marketId); | |
| } |
| migrator.MOOLAH().setAuthorization(address(migrator), true); | ||
| uint cdpDebt = migrator.migratePosition(slisBnb_marketParams, false); | ||
|
|
||
| // CDP postion should be cleared |
There was a problem hiding this comment.
Fix typos in identifier/comment text to improve readability and searchability.
215942a to
a89939d
Compare
fix: add validation that `cdpDebt > 0` before migration
📄 Description
Added
PositionMigratorwith account whitelist + supported-collateral configuration and flash-loan-based migration flow.🧠 Rationale
Users holding collateralized positions in CDP need a seamless way to migrate into
Moolahwithout manually unwinding and rebuilding their positions — a process that requires upfront capital, exposes them to price risk during the transition, and creates unnecessary friction.PositionMigratorsolves this by using a flash loan to atomically close the user's existing position and open an equivalent one in Moolah within a single transaction.🧪 Example / Testing
🧬 Changes Summary
Notable changes:
PositionMigrator