Feat/psm #2
Feat/psm #2
Conversation
0xtj24
commented
Jul 18, 2025
0xtj24
commented
- Add PSM
- Add Controller
- Add PSMFed
🛡️ Immunefi PR ReviewsThanks for submitting this PR for review! We’ve assigned 6 code reviewer(s) to take a look. They’ll provide feedback here as soon as possible. This review is based on the current state of your pull request. If you make changes after the review starts, they won’t be reflected here. To ensure the review includes your latest updates, you’ll need to open a new pull request. |
![immunefi-magnus[bot]](https://avatars.githubusercontent.com/in/1208986?s=60&v=4){kind=small}
| uint256 profit = amountOut > supply ? amountOut - supply : 0; | ||
| if (profit > 0) { | ||
| vault.withdraw(profit, gov, address(this)); | ||
| } |
| */ | ||
| function setSupplyCap(uint256 newSupplyCap) external onlyGov { | ||
| uint256 oldSupplyCap = supplyCap; | ||
| supplyCap = newSupplyCap; |
There was a problem hiding this comment.
add check that new supplyCap is >= supply
- Alzhan
There was a problem hiding this comment.
need to also be able to lower the cap below the supply so it cannot expand but only contract if gov decides to lower it
src/PSM.sol
Outdated
|
|
||
| collateral.safeTransferFrom(msg.sender, address(this), collateralAmountIn); | ||
| collateral.approve(address(vault), collateralAmountIn); | ||
| vault.deposit(collateralAmountIn, address(this)); |
There was a problem hiding this comment.
for extra safety might want to require > 0 shares minted. protects against edge case where vault is empty and a deposit is front run, to round it down to 0.
There was a problem hiding this comment.
Fixed in aa73378
Also added a check for minTotalSupply set by gov to ensure that a min shares have been minted to expand mitigation for an inflation attack
src/PSM.sol
Outdated
| */ | ||
| function buy(address to, uint256 collateralAmountIn) public { | ||
| require(collateralAmountIn > 0, "Amount must be > 0"); | ||
| require(controller.onBuy(msg.sender, collateralAmountIn), "Denied by controller"); |
There was a problem hiding this comment.
note: the to address is not also passed through to the controller for access control, or any arbitrary logic that may go into the controller.
src/PSM.sol
Outdated
| collateral.approve(address(vault), collateralAmountIn); | ||
| vault.deposit(collateralAmountIn, address(this)); | ||
| DOLA.safeTransfer(to, amountOut); | ||
| emit Buy(msg.sender, collateralAmountIn, amountOut); |
There was a problem hiding this comment.
ordering of event parameters is incorrect according to the definition.
event Buy(address indexed user, uint256 purchased, uint256 spent);
src/PSM.sol
Outdated
| function takeProfit() public { | ||
| uint256 vaultBal = vault.balanceOf(address(this)); | ||
| uint256 amountOut = vault.previewRedeem(vaultBal); | ||
| uint256 profit = amountOut > supply ? amountOut - supply : 0; |
There was a problem hiding this comment.
note: could simplify by making getProfit() function public and using here
src/PSM.sol
Outdated
| require(collateralBalance >= minCollateralAmount, "Insufficient collateral balance for migration"); | ||
| if (collateralBalance > 0) { | ||
| collateral.approve(address(vault), collateralBalance); | ||
| vault.deposit(collateralBalance, address(this)); |
There was a problem hiding this comment.
similar as above, could add a check for > 0 shares returned
| * @param minCollateralAmount Minimum amount of collateral to be deposited in the new vault. | ||
| * @param minSharesOut Minimum amount of shares to receive from the new vault. | ||
| */ | ||
| function migrate(address newVault, uint256 minCollateralAmount, uint256 minSharesOut) external onlyGov { |
There was a problem hiding this comment.
minTotalSupply check should apply here to the new vault
There was a problem hiding this comment.
In this case we already have a minSharesOut which fully mitigates an inflation attack or you refer we shouldn't migrate to a new vault if the minTotalSupply is not met?
|
LGTM |
