PSM #1

0xtj24 · 2025-06-08T18:59:46Z

No description provided.

08xmt · 2025-06-09T02:22:38Z

src/PSM.sol

+        _;
+    }
+
+    function buy(address to, uint256 amount) external {


May want to check maxDeposit for the ERC4626, though I don't think it's strictly necessary.

Imo it's not necessary, it would still revert on the vault if there's a limit (sUSDS has max uint256), if we avoid it we could save same gas (same for checking max withdraw). But can add it if you think it's better to have our custom revert.

src/PSM.sol

08xmt · 2025-06-09T02:33:50Z

src/PSM.sol

+        uint256 amountIn = amount;
+        if (depositFeeBps > 0) {
+            uint256 fee = (amount * depositFeeBps) / BPS_DENOMINATOR;
+            amountIn += fee;


Nitpick: I prefer subtracting fees from DOLA paid out to the buyer, than adding the fee to the amountIn, but it's not a hill I'm going to die on.

08xmt · 2025-06-09T02:37:07Z

src/PSM.sol

+    }
+
+    function sweep(IERC20 token) public onlyOperator {
+        require(token != IERC20(address(vault)), "Vault token cannot be swept");


Vault token should be sweepable for emergencies.

Fixed in f6de449

08xmt · 2025-06-09T03:11:02Z

src/PSM.sol

+    using SafeERC20 for IERC20;
+
+    IERC20 public immutable collateral;
+    IERC4626 public immutable vault;


Vaults should be setable and migrateable, so the PSM can be used to dogfood lending markets in the future.

src/Controller.sol

08xmt · 2025-06-14T10:41:41Z

src/PSM.sol

+        if (vault.balanceOf(address(this)) != 0) {
+            vault.redeem(vault.balanceOf(address(this)), address(this), address(this));
+        }


Migration will fail if redeem > maxWithdraw

If maxRedeem exceeds the balance in the PSM, should we allow to set a specific amount lower than that and allow for multiple smaller migrations?
In this edge case I guess we would then handle both vaults in terms of selling, total reserves and profit or we could just sweep the old vault tokens and redeem them via governance in multiple steps depending on availability (sending shares to the PSM)? Let me think a bit on what could be the side effects if any

08xmt · 2025-06-14T10:49:35Z

src/PSM.sol

        collateral.approve(address(vault), amountIn);
        vault.deposit(amountIn, address(this));


This pattern is a good example of why we shouldn't add fee to amountIn but rather subtract it from amount out.

Imagine a situation where we suddenly break integrations by increasing fee from 0 to 10 bps, because all approvals are now 10 bps short of the necessary amountIn

My understanding of the PSM was that integrators that need DOLA would use it to get the exact amount out of DOLA rather than a fixed amount in of collateral to get some amount of DOLA depending on fees.

The previous stabilizer only allowed to buy a specific amount of DOLA so kept the same pattern.

Anyway in both patterns if the deposit fee is changed it would require the integrator to use the view functions to adjust the approval of the collateral amount on the PSM to get the DOLA out that they require.
But if I misunderstood and the exact DOLA out is not what they need, can easily update that.

Fixed in d0be880

08xmt · 2025-06-14T10:59:26Z

src/PSM.sol

+    // Include profit and fees in total reserves
+    function getTotalReserves() public view returns (uint256) {
        return vault.previewRedeem(vault.balanceOf(address(this)));
    }

+    function getProfit() external view returns (uint256) {
+        return getTotalReserves() - supply;


Consider getting rid of the supply variable by doing the following:

Suggested change

// Include profit and fees in total reserves

function getTotalReserves() public view returns (uint256) {

return vault.previewRedeem(vault.balanceOf(address(this)));

}

function getProfit() external view returns (uint256) {

return getTotalReserves() - supply;

// Include profit and fees in total reserves

function getTotalReserves() public view returns (uint256) {

return vault.previewRedeem(vault.balanceOf(address(this))) + dola.balanceOf(address(this));

}

function getProfit() external view returns (uint256) {

return getTotalReserves() + dola.balanceOf(address(this)) - PSMFed.supply();

After trying to remove supply, realized that in case of a donation of DOLA to the PSM it would break the profit logic and we would have to sweep the donated DOLA in the best case scenario.
But in the case when takeProfit is called before we sweep, it will withdraw more USDS than the actual profit so we would end up having to first sweep the DOLA donated and then redeposit into the vault the excess USDS mistakenly taken as profit and send shares to the PSM.
It's more a griefing vector but could be annoying and repeatable.
Is there a specific reason to remove the supply ? Otherwise I would keep it

08xmt · 2025-06-14T11:00:48Z

src/PSM.sol

-    function setSupplyCap(uint256 newSupplyCap) external onlyOperator {
-        supplyCap = newSupplyCap;
-        emit SupplyCapUpdated(supplyCap);
+    function setGov(address newGov) external onlyGov {


Should use set pending -> accept pattern for gov change.

Fixed in 0ddf1dc

08xmt · 2025-06-14T11:02:20Z

src/PSMFed.sol

+
+contract PSMFed {
+    address public immutable psm;
+    address public immutable gov;


Gov should be migrateable

Fixed in 0ddf1dc

nourharidy · 2025-06-29T19:22:39Z

src/PSM.sol

+import {PSMFed} from "src/PSMFed.sol";
+
+interface IController {
+    function isBuyAllowed(uint256 amount) external view returns (bool);


should probably pass msg.sender to the controller also in case some rules can be added about the caller in the future

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:27:21Z

src/PSM.sol

+        address _vault,
+        address _DOLA,
+        address _gov,
+        uint256 _depositFeeBps,


To simplify, fees can be removed from the constructor in which case they will default to zero. In the governance proposal, governance can set the fees anyway

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:33:46Z

src/PSM.sol

+     * @param to Address to receive the collateral.
+     * @param amount Amount of DOLA to sell.
+     */
+    function sell(address to, uint256 amount) public {


I think dolaAmountIn is a clearer param here

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:34:03Z

src/PSM.sol

+     * @param to DOLA receiver.
+     * @param amountIn Amount of collateral to sell for DOLA.
+     */
+    function buy(address to, uint256 amountIn) public {


collateralAmountIn is clearer

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:42:32Z

src/PSM.sol

+        address oldVault = address(vault);
+        vault = IERC4626(newVault);
+        uint256 collateralBalance = collateral.balanceOf(address(this));
+        if (collateralBalance != 0) {


Actually, I would prefer to revert if collateralBalance is 0 in this step. Something isn't right if we're migrating but nothing ends up going into the new vault

The balance checks were introduced to be able to also call migrate when the migration has to be done manually via gov (in the unlikely case when there might be redemption or deposit limits in the vaults, fyi sUSDS has no limits).
See test here

PSM/test/PSMUSDSFork.t.sol

Line 256 in 6dd04c9

function test_migrate_manually_via_gov_if_maxRedeem_lower_than_psm_balance() public {

It was done to avoid having another function that just takeProfit and update the vault.
But if you think it’s better to have a separate function I can easily update that.

In this case, we should add a minCollateralMigrated param for the caller (gov)

nourharidy · 2025-06-29T19:44:26Z

src/Controller.sol

+     * @return bool Returns true if the buy operation is allowed, false otherwise.
+     * For now, it returns true to allow all buy operations.
+     */
+    function isBuyAllowed(uint256 amount) external pure returns (bool) {


Make sure to update this contract if you change the interface in the other file as I suggested

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:49:31Z

src/Controller.sol

+     * @return bool Returns true if the buy operation is allowed, false otherwise.
+     * For now, it returns true to allow all buy operations.
+     */
+    function isBuyAllowed(uint256 amount) external pure returns (bool) {


Controller functions are better off being mutable functions (not pure or view) because we might want to store state in this contract e.g. in the case of daily in/outflow limits etc. In this case, we should probably also rename the functions to onBuy and onSell since they become hooks.

Also please lets make sure we're consistent in naming. In some places, we seem to be using sell and withdraw, buy and deposit interchangeably. Lets pick one. I think buy and sell are clearer.

Fixed in 6dd04c9

nourharidy · 2025-06-29T19:57:19Z

src/PSMFed.sol

+     * @param newChair Address of the new chair.
+     */
+    function setChair(address newChair) external onlyGov {
+        require(newChair != address(0), "Invalid address");


Maybe it should be 0 (no chair). Either way, no risk here in cases of mistakes so checks aren't necessary

Fixed in 6dd04c9

src/Controller.sol

nourharidy · 2025-07-03T16:43:48Z

src/PSM.sol

+        uint256 amountOut = collateralAmountIn;
+
+        if (depositFeeBps > 0) {
+            uint256 fee = (collateralAmountIn * depositFeeBps) / BPS_DENOMINATOR;


We're still using depositFee and withdrawalFee naming here and in other places when the function is called buy. Let's rename it to buyFee and sellFee for consistency

Fixed in 08ddd69 and following

nourharidy · 2025-07-03T16:45:39Z

src/PSM.sol

+        address oldVault = address(vault);
+        vault = IERC4626(newVault);
+        uint256 collateralBalance = collateral.balanceOf(address(this));
+        if (collateralBalance != 0) {


In this case, we should add a minCollateralMigrated param for the caller (gov)

nourharidy · 2025-07-10T05:16:32Z

src/PSM.sol

+     */
+    function sell(address to, uint256 dolaAmountIn) public {
+        require(dolaAmountIn > 0, "Amount must be > 0");
+        require(controller.onBuy(msg.sender, dolaAmountIn), "Denied by controller");


using onBuy instead of onSell here. Caught by AI.

Fixed in 485612f

nourharidy · 2025-07-10T05:20:55Z

src/PSM.sol

+        address oldVault = address(vault);
+        vault = IERC4626(newVault);
+        uint256 collateralBalance = collateral.balanceOf(address(this));
+        if (collateralBalance >= minCollateralAmount) {


The alternative needs to be a revert. So in this case we need to switch this to a require instead of an if statement

Fixed in 4a419d5

0xtj24 added 2 commits June 8, 2025 13:57

add PSM

bbd52b2

run formatter

70b03ba

08xmt reviewed Jun 9, 2025

View reviewed changes

0xtj24 added 2 commits June 13, 2025 15:22

add PSMFed and Controller, update tests

f6de449

format

95b355a

08xmt reviewed Jun 14, 2025

View reviewed changes

src/Controller.sol Show resolved Hide resolved

08xmt reviewed Jun 14, 2025

View reviewed changes

0xtj24 added 10 commits June 17, 2025 16:50

add migratable gov and controller update

0ddf1dc

fix test

fe9a61f

add total reserve test with fee and profit

4989fdd

update buy for collateralIn and deposit fee logic, add natspec

d0be880

format

fe88039

add test for manual migration via gov

8c99fe4

add sUSDS test, update DOLA interface

ff9edfa

update test.yml

103fd28

use secret in test

af9fd95

update test.yml

d200808

nourharidy requested changes Jun 29, 2025

View reviewed changes

update after feedback

6dd04c9

nourharidy reviewed Jul 3, 2025

View reviewed changes

0xtj24 added 5 commits July 3, 2025 14:28

rename fees, add warning

08ddd69

format

e1b7f83

rename fee events

7e8779e

fee renaming

2396c43

add minCollateralAmount param in migrate

dbc9305

nourharidy requested changes Jul 10, 2025

View reviewed changes

0xtj24 added 2 commits July 10, 2025 15:32

use onSell

485612f

revert if below minCollateralOut

4a419d5

0xtj24 closed this Jul 18, 2025

		collateral.approve(address(vault), amountIn);
		vault.deposit(amountIn, address(this));

PSM #1

PSM #1

Uh oh!

Conversation

0xtj24 commented Jun 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xtj24 Jun 17, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

0xtj24 Jun 17, 2025 •

edited

Loading