Wrapped OUSD #957
Wrapped OUSD #957
Conversation
| * @notice Show the total amount of OUSD held by the wrapper | ||
| */ | ||
| function totalAssets() public view override returns (uint256) { | ||
| return ERC20(asset).balanceOf(address(this)); |
There was a problem hiding this comment.
this is possibly manipulatable in a cream-hack esque manner no? would make wOUSD unsuitable for borrowing in lending markets
There was a problem hiding this comment.
I believe we're saved here by the fact that the protocol will never buy OUSD for more than $1, or sell it for less than
More generally, the best practice is to always use a dedicated oracle for the borrowed asset instead of a derived one to prevent shenanigans with bendy bonding curves.
There was a problem hiding this comment.
The issue is that you can flat out give OUSD to wOUSD, making each wOUSD worth more. Giving away money like this is a loss for the attacker - wOUSD holders, and OUSD holders both are happy and good here. The problem is if a lending platform is using wOUSD as collateral, has lended it back to the attacker and suddenly thinks the attacker can borrow a bajillion dollars.
See these two threads:
- https://twitter.com/danielvf/status/1453426746326405130
- https://twitter.com/danielvf/status/1453460110760677379
This isn't a vulnerability in wOUSD, just something a lending platform has to take into account because the price of WOUSD going up could take place inside the same transaction.
There was a problem hiding this comment.
Started reviewing. I will dig further in the ERC4626 implementation.
I'd vote for making wOUSD upgradeable even if it means the operations cost a little bit more gas (should be in the order of an extra 1k gas per call, isn't it?). To give us the peace of mind that we can upgrade in case of an emergency.
|
note that making it upgradable will require writing your own ERC4626 impl as solmate is not designed for upgradability |
|
@franckc To answer your gas question, a proxy adds about 5-6K gas per call. For comparison, a partial transfer between two accounts that each already hold wOUSD is 34,210 gas with no proxy. |
|
@tomlinton Can you take a look? |
|
Where does this OpenZeppelin ERC4626 implementation come from? I assume it is not from an official release which is why we've added it to the repo instead of just referencing the installed dependency. I think we should add a comment at the time of the OZ files indicating the source and the commit hash if any. |
|
Comes from the draft PR here: Given that it is a draft, it's worth looking hard at. |
|
Added a test for upgrading. |
|
Not sure if we are waiting on my approval for this, but if we are, I'm still insisting on that commit hash for the draft ERC4626 implementation to be added as a comment :P |
Done! |
This adds wrapped OUSD, a non rebasing, but yield earning, contract for putting OUSD into. Closes #876
This is based on the solmate ERC4626 implementation. This implementation is in use by Tribe for their new Turbo Safes, but is not widely used yet. I think using a prebuilt ERC4626 implementation is a fantastic way to go here. The standard is a perfect match for a wrapper token, and the rounding and other details has seen a bit of community scrutiny. According to Joey with tribe, it was audited by a C4 contest and spearbit, however neither report is public at this time. This code has not yet been audited to the same level as OpenZeppelin contracts. That said, I think it's more refined, and has seen more community review, than something I would have written scratch here.
So we should also take a look through the base ERC4626 contract when reviewing this PR
I've tested this by making hundreds of paired test transactions, and ensuring the rounding is sane, and holders earnings follow the rebasing correctly.
Some decisions that are up for debate:
Note 1: Because of the simplicity and elegantness of this implementation, if someone sends this contract OUSD, they aren't getting it back, and it is instantly yield for everyone holding OUSD in the contract.
Note 2: In the past when people have wanted to
rebaseOptInfrom their contracts, we've told them they would need to do it after the constructor. Turns out there's a way around that. :DContract change checklist: