Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapped OUSD #957

Merged
merged 13 commits into from
Apr 11, 2022
Merged

Wrapped OUSD #957

merged 13 commits into from
Apr 11, 2022

Conversation

DanielVF
Copy link
Collaborator

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:

  1. I've imported the solmate library contracts into our repo. Their last NPM release was a while ago, and does not have the ERC4626 implementation. I think they will be making an NPM release, possibly before this PR is merged, and we can switch back to using that then.
  2. I've gone for a non-upgradable contract. Its cheaper gas, less can go wrong. I'm up for alternative views on this one.
  3. I added transferToken to this. One day when someone mistakenly sends 600,000 USDC to this contract, it can be recovered. This also mean that wousd has to have a governor, and governor/ownership methods. Added.

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 rebaseOptIn from their contracts, we've told them they would need to do it after the constructor. Turns out there's a way around that. :D

Contract change checklist:

  • Code reviewed by 2 reviewers. See template and example.
  • Unit tests pass
  • Slither tests pass with no warning
  • Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

* @notice Show the total amount of OUSD held by the wrapper
*/
function totalAssets() public view override returns (uint256) {
return ERC20(asset).balanceOf(address(this));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is possibly manipulatable in a cream-hack esque manner no? would make wOUSD unsuitable for borrowing in lending markets

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 $1. Otherwise, you're right, you'd be able to temporarily bend the price of OUSD, buy up a bunch at $.90 then swap it for wOUSD before redeeming it at full price.

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.

Copy link
Collaborator Author

@DanielVF DanielVF Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Copy link

@franckc franckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

contracts/contracts/token/WrappedOusd.sol Outdated Show resolved Hide resolved
@transmissions11
Copy link

note that making it upgradable will require writing your own ERC4626 impl as solmate is not designed for upgradability

@DanielVF
Copy link
Collaborator Author

@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.

Copy link

@franckc franckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed focusing on the contracts.
ERC4626's implementation is really clean.
Looks very solid to me.

LGTM

@DanielVF
Copy link
Collaborator Author

@tomlinton Can you take a look?

contracts/package.json Outdated Show resolved Hide resolved
@tomlinton
Copy link
Contributor

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.

Copy link

@franckc franckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a test to make sure we can upgrade after there has been some activity on the wousd contract. Then checking the balances are right after the upgrade.

@DanielVF
Copy link
Collaborator Author

Comes from the draft PR here:

Given that it is a draft, it's worth looking hard at.

OpenZeppelin/openzeppelin-contracts#3171

@DanielVF
Copy link
Collaborator Author

DanielVF commented Apr 1, 2022

Added a test for upgrading.

@tomlinton
Copy link
Contributor

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

@DanielVF
Copy link
Collaborator Author

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!

@DanielVF DanielVF merged commit f555a94 into master Apr 11, 2022
@DanielVF DanielVF deleted the DanielVF/wousd branch April 11, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OUSD wrapper contract
5 participants