feat: add MGP17 by nvtaveras · Pull Request #56 · mento-protocol/deployments-v2

nvtaveras · 2026-04-15T11:20:36Z

No description provided.

cursor

Stale comment

This PR adds BiPoolManagerFeeSetter implementation addresses plus a governance proposal/script to upgrade BiPoolManager so spread fees can be changed without recreating pools.

mgps/mgp17.md

Important: L3-L4 says this proposal starts by reducing selected USDm pairs from 5 bps to 2 bps, but L40-L46 and script/migration/MGP17.sol only perform an implementation upgrade. There are no setSpread calls in the payload, so the prose overstates what governance is actually approving.

script/migration/MGP17.sol

Important: L56-L60 only verify proxy-admin ownership. This upgrade adds a new owner-gated capability, but the script never asserts that IOwnable(biPoolManagerProxy).owner() is the intended fee-setting authority. checkSetSpreadPermission() then pranks whatever address owner() returns, so the proposal can pass while silently granting spread control to an unexpected account.

Important: L82-L85 and L88-L101 prove setSpread on one arbitrary exchange (exchangeIds[0]) instead of the USDm collateral pools the proposal is meant to unblock. If the regression is specific to USDm/USDC, USDm/axlUSDC, or USDm/USDT, this post-check still passes.

Final verdict: 🚫 Needs changes

_{Sent by Cursor Automation: PR Review}

cursor · 2026-04-15T11:26:06Z

@@ -0,0 +1,51 @@
+## TL;DR
+
+We propose upgrading the BiPoolManager implementation to a new version that adds a `setSpread` function. The currently deployed implementation requires destroying and recreating a pool to change its fee, making fee adjustments operationally expensive. This upgrade enables the contract owner to adjust spread fees directly, starting with a reduction from 5bps to 2bps on select USDm pairs.


L3-L4 says this proposal starts by reducing spreads from 5 bps to 2 bps on selected USDm pairs, but the executable payload below only upgrades the implementation. There are no setSpread calls anywhere in script/migration/MGP17.sol, so this TL;DR is describing an effect governance is not actually approving in this proposal.

cursor · 2026-04-15T11:26:07Z

+
+    function preChecks() internal view {
+        console.log("== Pre-checks ==");
+        console.log(unicode" > 👀 checking BiPoolManager proxy admin is governance timelock");


This pre-check is missing the more important invariant for this upgrade: who currently owns BiPoolManager. setSpread is owner-gated, and checkSetSpreadPermission() later pranks whatever owner() returns, so the script will still pass even if spread-setting authority is already held by the wrong account. For a proposal that introduces new privileged behavior, you should assert the expected owner() here before upgrading.

cursor · 2026-04-15T11:26:07Z

+
+        address owner = IOwnable(biPoolManagerProxy).owner();
+        bytes32[] memory exchangeIds = IBiPoolManager(biPoolManagerProxy).getExchangeIds();
+        require(exchangeIds.length > 0, "no exchanges found on BiPoolManager");


This only exercises setSpread on exchangeIds[0], which is too weak for the stated goal of enabling fee changes on the USDm collateral pools. If the upgraded implementation regresses only on USDm/USDC, USDm/axlUSDC, or USDm/USDT, this check still passes. Target the intended pools explicitly or iterate all exchanges instead of relying on the first entry.

cursor

Stale comment

Adds MGP-17 governance artifacts to upgrade BiPoolManager to a setSpread-capable implementation and records the new implementation address for mainnet and Sepolia.

mgps/mgp17.md

Important

:3, :15-20 The proposal text says this change starts reducing spreads from 5bps to 2bps, but script/migration/MGP17.sol only upgrades the implementation. There is no setSpread call in the proposal. Reword the TL;DR/overview so voters are not told this proposal changes live pool fees when it only enables a later owner action.

script/migration/MGP17.sol

Important

:83-101 checkSetSpreadPermission() uses sender("migrationOwner").account as the source of truth for ownership. That makes the post-check depend on local Treb sender config instead of the upgraded contract state, and it never explicitly validates owner() after the upgrade. Read IOwnable(biPoolManagerProxy).owner() from chain state and prank that address for the capability check; if you also want config validation, assert the on-chain owner matches the expected sender separately.

Final verdict: 🚫 Needs changes

_{Sent by Cursor Automation: PR Review}

cursor · 2026-04-15T11:49:34Z

@@ -0,0 +1,51 @@
+## TL;DR
+
+We propose upgrading the BiPoolManager implementation to a new version that adds a `setSpread` function. The currently deployed implementation requires destroying and recreating a pool to change its fee, making fee adjustments operationally expensive. This upgrade enables the contract owner to adjust spread fees directly, starting with a reduction from 5bps to 2bps on select USDm pairs.


This reads as if MGP-17 itself reduces live spreads to 2bps, but script/migration/MGP17.sol only upgrades the implementation. Because governance voters rely on this markdown to understand what will execute, please reword the TL;DR/overview to make it clear that this proposal only enables a later fee change.

cursor · 2026-04-15T11:49:34Z

+        address owner = sender("migrationOwner").account;
+        bytes32[] memory exchangeIds = IBiPoolManager(biPoolManagerProxy).getExchangeIds();
+        require(exchangeIds.length > 0, "no exchanges found on BiPoolManager");
+
+        bytes32 exchangeId = exchangeIds[0];
+        IBiPoolManager.PoolExchange memory exchange = IBiPoolManager(biPoolManagerProxy).getPoolExchange(exchangeId);
+        uint256 originalSpread = exchange.config.spread.value;
+
+        // Set a test spread value
+        uint256 testSpread = originalSpread == 0 ? 1 : originalSpread - 1;
+        vm.prank(owner);
+        IBiPoolManager(biPoolManagerProxy).setSpread(exchangeId, testSpread);


This post-check is proving that the locally configured migrationOwner sender can call setSpread, not that the upgraded contract preserved the correct owner. That makes the result namespace-config dependent and misses an explicit owner() assertion after the upgrade. Please read IOwnable(biPoolManagerProxy).owner() from chain state, prank that address for the capability check, and only compare it to migrationOwner as a separate config sanity check if needed.

cursor

Stale comment

This PR adds the MGP-17 proposal artifacts and the migration script to upgrade BiPoolManager to an implementation that exposes setSpread. I reviewed all six changed files; only the files below have findings.

mgps/mgp17.md

🔴 Critical: 50-51 say the proposal upgrades proxy 0x22d9db95E6Ae61c104A7B6F6C78D7993B94ec901 to implementation 0xC016174B60519Bdc24433d4ed2cFf6c1efaC7881, but the only committed broadcast payload in this PR upgrades 0xeCB3C656C131fCd9bB8D1d80898716bD684feb78 to 0x15b38D560928029a74d1D0a56eE232fb5Ff3A4B1 (broadcast/MGP17.sol/11142220/run-1776254228027.json:10-13, broadcast/MGP17.sol/11142220/run-latest.json:10-13). The human-readable proposal text and the executable proposal artifacts describe different actions, so reviewers cannot tell which contracts this PR is actually proposing to upgrade.

broadcast/MGP17.sol/11142220/run-1776254228027.json

🔴 Critical: 10-13 encode a proposal that targets the Sepolia proxy/implementation, while the embedded markdown still names the mainnet proxy/implementation. If this file is meant to be the auditable rehearsal artifact, it is currently misleading.

broadcast/MGP17.sol/11142220/run-latest.json

🔴 Critical: same issue at 10-13; the committed run-latest artifact disagrees with the proposal text about the target proxy and implementation.

.treb/deployments.json

🟡 Important: 25786-25789 update the proxy's current implementation to 0x15b38D560928029a74d1D0a56eE232fb5Ff3A4B1 but still record the upgrade history as testnet-v2-rc5/11142220/BiPoolManager:v2.6.5, whose deployment entry resolves to 0x641A91B3c70D0E4A9A8bf5B3a2047ed9762F600c (22089-22095) rather than 0x15b38.... Any tooling that follows implementationId will now point at the wrong artifact/address for this upgrade.

Final verdict: 🚫 Needs changes

_{Sent by Cursor Automation: PR Review}

cursor · 2026-04-15T12:45:21Z

+
+| Contract      | Address                                    |
+| ------------- | ------------------------------------------ |
+| BiPoolManager (proxy) | 0x22d9db95E6Ae61c104A7B6F6C78D7993B94ec901 |


These addresses do not match the committed proposal payload. The checked-in broadcasts target 0xeCB3C656C131fCd9bB8D1d80898716bD684feb78 and 0x15b38D560928029a74d1D0a56eE232fb5Ff3A4B1, so either this markdown or the broadcast artifacts are wrong. As written, reviewers are being shown different contracts than the ones the proposal actually encodes.

cursor · 2026-04-15T12:45:21Z

+        "[0xeCB3C656C131fCd9bB8D1d80898716bD684feb78]",
+        "[0]",
+        "[0xbb913f4100000000000000000000000015b38d560928029a74d1d0a56ee232fb5ff3a4b1]",
+        "{\\\"description\\\":\\\"## TL;DR\\\\n\\\\nWe propose upgrading the BiPoolManager implementation to a new version that adds a `setSpread` function. The currently deployed implementation requires destroying and recreating a pool to change its fee, making fee adjustments operationally expensive. This upgrade enables the contract owner to adjust spread fees directly, starting with a reduction from 5bps to 2bps on select USDm pairs.\\\\n\\\\n---\\\\n\\\\n## Overview\\\\n\\\\nThe currently deployed BiPoolManager implementation does not have a `setSpread` function, meaning every fee change requires destroying and recreating the pool with the new fee parameters. This proposal upgrades the BiPoolManager proxy to a new implementation that adds a `setSpread` function, allowing the contract owner to adjust spread fees directly without the overhead of pool destruction and recreation.\\\\n\\\\nThe motivation behind this change is to enable fee structure experimentation on Mento collateral pools. The previous 5bps spread fee, introduced via MGP-13, was set to protect reserves from arbitrage losses and generate protocol revenue. However, Mento Labs aims to optimize the fee structure by balancing reserve protection and protocol revenue with keeping Mento competitive in on-chain stablecoin swaps.\\\\n\\\\n### Initial Fee Adjustment\\\\n\\\\nFollowing this upgrade, Mento Labs plans to reduce spread fees from 5bps to 2bps on:\\\\n\\\\n- USDm to USDC\\\\n- USDm to axlUSDC\\\\n- USDm to USDT\\\\n\\\\nOngoing monitoring of swap volume, reserve impact, and protocol revenue will inform further adjustments, communicated via the forum.\\\\n\\\\nFor full details, see the forum discussion: [Fee Structure Experimentation on Mento Collateral Pools](https://forum.mento.org/t/fee-structure-experimentation-on-mento-collateral-pools/115)\\\\n\\\\n---\\\\n\\\\n### Security Considerations\\\\n\\\\n**Risk Assessment**\\\\n\\\\n- The upgrade only changes the BiPoolManager implementation; no ownership transfers are involved.\\\\n- The governance timelock retains proxy admin ownership of the BiPoolManager.\\\\n\\\\n**Safety Measures**\\\\n\\\\nThe new implementation is the same one that was used in [MGP-13: Increase Circuit Breaker to 15bps and Enable 5bps Spread Fee](https://governance.mento.org/proposals/33774473826582746102247872468112299588808192529529153992548885265611575112929), which has already been reviewed and executed successfully. The proxy upgrade mechanism is the same Celo legacy proxy pattern used across all existing Mento V2 contracts.\\\\n\\\\n### Transaction Details\\\\n\\\\nThis proposal consists of **1 transaction**.\\\\n\\\\n**Step 1: Upgrade BiPoolManager Implementation (1 transaction)**\\\\n\\\\nUpgrade the BiPoolManager proxy to the new implementation that adds the `setSpread` function.\\\\n\\\\n- Call `_setImplementation(address)` on the BiPoolManager proxy\\\\n\\\\n| Contract      | Address                                    |\\\\n| ------------- | ------------------------------------------ |\\\\n| BiPoolManager (proxy) | 0x22d9db95E6Ae61c104A7B6F6C78D7993B94ec901 |\\\\n| New Implementation    | 0xC016174B60519Bdc24433d4ed2cFf6c1efaC7881 |\\\\n\\\",\\\"title\\\":\\\"MGP-17: Upgrade BiPoolManager with FeeSetter\\\"}"


This JSON encodes a proposal targeting the Sepolia proxy/implementation from lines 10-12, but the embedded markdown string still says the upgrade is for 0x22d9... -> 0xC016.... That makes the committed rehearsal artifact internally inconsistent and misleading to review.

cursor

This PR adds the MGP-17 proposal artifacts and migration script to upgrade BiPoolManager to an implementation that exposes setSpread. I reviewed all 7 changed files; only the files below have findings.

mgps/mgp17.md

🟡 Important: 3, 15-19 say this proposal starts by reducing selected USDm collateral-pool spreads from 5bps to 2bps, but 42-46 describe only a single _setImplementation(address) call. The executable payload never calls setSpread, so the human-readable proposal text overstates what governance is actually approving.

script/migration/MGP17.sol

🟡 Important: 83-94 proves the new permission against sender("migrationOwner").account, not against the upgraded contract's actual on-chain owner(). That makes the post-check depend on local Treb sender configuration instead of the state the proposal leaves behind; if those ever diverge, this check can pass or fail for the wrong reason. Read IOwnable(biPoolManagerProxy).owner() from chain state and, if desired, assert separately that it matches the expected sender alias.

broadcast/MGP17.sol/42220/run-1776267061591.json

🟡 Important: 13 embeds the same misleading proposal text as mgps/mgp17.md: it says the proposal starts lowering live spreads, but the encoded calldata only submits the _setImplementation(address) action.

broadcast/MGP17.sol/42220/run-latest.json

🟡 Important: 13 has the same issue as the timestamped mainnet artifact: the embedded description promises a fee reduction that the calldata does not perform.

broadcast/MGP17.sol/11142220/run-1776254228027.json

🟡 Important: 13 is self-contradictory. The actual Sepolia proposal targets proxy 0xeCB3C656C131fCd9bB8D1d80898716bD684feb78 at 10 and implementation 0x15b38D560928029a74d1D0a56eE232fb5Ff3A4B1 at 12, but the embedded markdown still says the proposal upgrades mainnet proxy 0x22d9db95E6Ae61c104A7B6F6C78D7993B94ec901 to implementation 0xC016174B60519Bdc24433d4ed2cFf6c1efaC7881. That makes the rehearsal artifact misleading.

broadcast/MGP17.sol/11142220/run-latest.json

🟡 Important: same issue at 10-13; the committed run-latest Sepolia artifact disagrees with itself about which proxy and implementation are being proposed.

Final verdict: 🚫 Needs changes

_{Sent by Cursor Automation: PR Review}

feat: add MGP17

4f05ac0

cursor bot requested changes Apr 15, 2026

View reviewed changes

feat: use migrationOwner for checks

b6ac83c

cursor bot requested changes Apr 15, 2026

View reviewed changes

feat: add MGP17 sepolia jsons

a489521

cursor bot requested changes Apr 15, 2026

View reviewed changes

philbow61 approved these changes Apr 15, 2026

View reviewed changes

feat: add MGP17 mainnet jsons

6b514b9

cursor bot requested changes Apr 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add MGP17#56

feat: add MGP17#56
nvtaveras wants to merge 4 commits intomainfrom
feat/MGP-17

nvtaveras commented Apr 15, 2026

Uh oh!

cursor bot left a comment •

edited

Loading

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot left a comment •

edited

Loading

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot left a comment •

edited

Loading

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot Apr 15, 2026

Uh oh!

cursor bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,51 @@
		## TL;DR

		We propose upgrading the BiPoolManager implementation to a new version that adds a `setSpread` function. The currently deployed implementation requires destroying and recreating a pool to change its fee, making fee adjustments operationally expensive. This upgrade enables the contract owner to adjust spread fees directly, starting with a reduction from 5bps to 2bps on select USDm pairs.

Conversation

nvtaveras commented Apr 15, 2026

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cursor bot left a comment •

edited

Loading

cursor bot left a comment •

edited

Loading

cursor bot left a comment •

edited

Loading