Refactor internal construction of ERC7540Deposit and ERC7540Redeem - #60
Refactor internal construction of ERC7540Deposit and ERC7540Redeem#60Amxx wants to merge 6 commits into
Conversation
|
| _transferIn(owner, assets); | ||
| emit DepositRequest(controller, owner, requestId, _msgSender(), assets); | ||
| return requestId; | ||
|
|
||
| return _requestDeposit(assets, controller, owner); |
There was a problem hiding this comment.
This breaks CI. I understand we assume the token is an ERC20 and is trusted, but I believe we should do:
uint256 requestId = _requestDeposit(assets, controller, owner);
_transferIn(owner, assets);
return requestId;This would cause totalAssets() to be understated but I think it's a safe direction (and already documented in the NatSpec note
There was a problem hiding this comment.
(I'm guessing you meant CEI as in check-effect-interact, and not CI as in continuous integration)
AFAIK, doing _transferIn at the very begining is fine, in the sens that its like if both operation were batched. CEI would be broken if we did checks before _transferIn, that the _transferIn can break. In this case the only part that is done before the _transferIn is the onlyOperatorOrController part.
But I'd be ok with moving the transfer to the end of the function if you think its better that way.
| * otherwise pending assets would be treated as yield for outstanding shares. | ||
| */ | ||
| function totalSupply() public view virtual override returns (uint256) { | ||
| return super.totalSupply() + totalPendingRedeemShares(); |
There was a problem hiding this comment.
Doesn't this include shares that don't actually exist? I feel that's wrong
There was a problem hiding this comment.
it includes the share that were burnt during the requestRedeem. The assets corresponding to which are still in the vault. The goal of this is to "trick" convertToAsset/convertToShare to give a correct rate.
But yes, it makes it that the totalSupply is not the sum of all the balances.
| function totalAssets() public view virtual override returns (uint256) { | ||
| return super.totalAssets() - totalPendingDepositAssets(); |
There was a problem hiding this comment.
Between requestDeposit and deposit, totalAssets is inflated while totalSupply hasn't changed. The feels bad because:
convertToAssets(shares)returns a higher value for existing shareholders- Any operation reading the exchange rate during this window sees an artificially high share price
- If the vault has a synchronous redeem path (e.g., it only uses ERC7540Deposit for async deposits but keeps sync redeems), an attacker could deposit a large amount via requestDeposit to inflate the rate, then immediately redeem existing shares at the inflated price
It also diverges from the production implementations we've researched. All of them exclude pending assets from totalAssets
There was a problem hiding this comment.
This function was moved, not removed.
There was a problem hiding this comment.
Note: I 100% agree this function is needed. The totalSupply override in ERC7540Redeem is basically the counterpart to this for handling shares being burnt before the assets are moved out.
| _totalPendingDepositAssets += assets; | ||
|
|
||
| emit DepositRequest(controller, owner, 0, _msgSender(), assets); | ||
| return 0; |
There was a problem hiding this comment.
Although I agree customizing the requestId will still require multiple overrides, the previous implementation is more clear imo: override _requestId + override storage getters/setters. If we go this route, then implementing epoch-based vaults becomes even more difficult. A developer will need to add back all of the requestId handling logic.
From the research we worked in the past weeks, all epoch-based implementations do use the requestId to identify the epoch (lagoon, cove, amphor). Removing _depositRequestId and _redeemRequestId them forces epoch implementations to rebuild the entire thing from scratch rather than extending from it.
| if (owner != sender && !isOperator(owner, sender)) { | ||
| _spendAllowance(owner, sender, shares); | ||
| } | ||
| _burn(owner, shares); |
There was a problem hiding this comment.
Related to https://github.com/ernestognw/openzeppelin-contracts/pull/60/changes#r3044978238
It also diverges from the implementations researched:
- BeefySonic, MagmaV2, Tangle: shares transferred to vault, burned at fulfillment
- Centrifuge: shares transferred to an escrow/vault contract
- All epoch-based implementations: shares held during the epoch, settled at epoch close
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
2 participants
ERC7540Deposit
deposit/mint. This changes the meaning of_totalPendingDepositAssets. IMO This is aligned with the lifecycle documented in the ERCremove requestId from the implementation, only supporting the values
0by default. Supporting more values would require many overrides by the user anyway.split public/internal function to guide users in their customization of the vault.
_fulfillDepositdoesn't get exchange rate from a function. Since this rate is likelly to depending on many parameters, it is expected that the public function that will execute the internal fulfill call will itself compute the rate at which the operation is being fulfilled, and will feed that data to the internal function through the args.ERC7540Redeem
(overall similar changes as above)
totalPendingRedeemSharessimilar to what exists in ERC7540Deposit to track shares that are burned but have not yet been redeemed.