Update last total assets in accept cap #1
Conversation
src/MetaMorpho.sol
Outdated
| @@ -779,6 +781,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph | |||
| } | |||
|
|
|||
| marketConfig.enabled = true; | |||
|
|
|||
| _updateLastTotalAssets(totalAssets()); | |||
There was a problem hiding this comment.
| _updateLastTotalAssets(totalAssets()); | |
| _updateLastTotalAssets(newTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))); |
And we could expect _setCap to have MarketParams as input (and in turn expect acceptCap) to have MarketParams as input
There was a problem hiding this comment.
Note that it changes the ABI, so we must notify @julien-devatom if we do it
There was a problem hiding this comment.
I feel like _updateLastTotalAssets(totalAssets()); does the same
There was a problem hiding this comment.
Or we could just not accrue fees and only have _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))), this might be the better option no ?
And we could use idToMarketParams so no need for _setCap to have MarketParams
There was a problem hiding this comment.
This would symmetrical to update withdraw queue indeed 👍
However I push for having MarketParams as input because we can afford to have it in this context, whereas we can't in ERC4626 functions (and is this the reason we use idToMarketParams)
There was a problem hiding this comment.
So Do I add MarketParams as input for acceptCap and submitMarketRemoval ? Or do I just change the Id input by MarketParams ?
There was a problem hiding this comment.
To me the id can always be calculated given the market params so i think only the market params is necessary
I'd wait for others' thoughts beforehand thoug
There was a problem hiding this comment.
Note that it changes the ABI, so we must notify @julien-devatom if we do it
Not only me, all the integrators !! It's also critical for integrators (can break everything)
There was a problem hiding this comment.
What do you mean by "can break everything", the fee accounting you mean?
| MarketParams memory marketParams = MORPHO.idToMarketParams(id); | ||
| _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))); |
There was a problem hiding this comment.
| MarketParams memory marketParams = MORPHO.idToMarketParams(id); | |
| _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))); | |
| _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(_marketParams(id), address(this))); |
| MarketParams memory marketParams = MORPHO.idToMarketParams(id); | ||
| _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))); |
There was a problem hiding this comment.
This behavior implies that no fee is capture when a non-zero supply market is opened
Which is the behavior we want I believe, but may not be canonical to curators
| morpho.borrow(allMarkets[0], deposited, 0, BORROWER, BORROWER); | ||
| vm.stopPrank(); | ||
|
|
||
| _forward(blocks); |
There was a problem hiding this comment.
This seems innocent, but it's not! Great
| @@ -279,4 +280,39 @@ contract MarketTest is IntegrationTest { | |||
| ); | |||
| vault.updateWithdrawQueue(indexes); | |||
| } | |||
|
|
|||
| function testenableMarketWithLiquidity(uint256 deposited, uint256 additionalSupply, uint256 blocks) public { | |||
There was a problem hiding this comment.
| function testenableMarketWithLiquidity(uint256 deposited, uint256 additionalSupply, uint256 blocks) public { | |
| function testEnableMarketWithLiquidity(uint256 deposited, uint256 additionalSupply, uint256 blocks) public { |
Fixes https://cantina.xyz/ai/8409a0ce-6c21-4cc9-8ef2-bd77ce7425af/findings/8b4fe500-afc7-43a2-9c14-d2ca13c5ae0a