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.

Sign up for GitHub

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

Jump to bottom

WOETH: Withdraw OETH in surplus. #2119

Open
wants to merge 11 commits into
base: sparrowDom/woeth_hack_proof
Choose a base branch
from
Open

WOETH: Withdraw OETH in surplus. #2119

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7f6795a
feat: withdraw surplus of oETH in wrapper.
clement-ux Jul 3, 2024
4b10871
test: add tests for surplus withdrawal.
clement-ux Jul 3, 2024
c3e83c4
fix: add visibility to internal function.
clement-ux Jul 3, 2024
429cb14
test: add test for rebase with loss.
clement-ux Jul 3, 2024
21ea003
style: run prettier.
clement-ux Jul 3, 2024
df78c6d
style: remove comment.
clement-ux Jul 3, 2024
9325358
perf: reduce `require` string lenght.
clement-ux Jul 5, 2024
09d4a44
fix: apply change on reverting string message to test.
clement-ux Jul 5, 2024
e192d51
test: cannot transfer more than surplus after rebase.
clement-ux Jul 5, 2024
afdccc1
style: run prettier.
clement-ux Jul 5, 2024
4a12161
test: remove test for negativ rebase.
clement-ux Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions contracts/contracts/token/WOETH.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ contract WOETH is ERC4626, Governable, Initializable {
external
onlyGovernor
{
//@dev TODO: we could implement a feature where if anyone sends OETH direclty to
// the contract, that we can let the governor transfer the excess of the token.
require(asset_ != address(asset()), "Cannot collect OETH");
if (asset_ == address(asset())) {
uint256 surplus = OETH(asset()).balanceOf(address(this)) - totalAssets();
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a quick look, I'm a bit lost here. OETH is a rebasing token. wOETH's appreciation comes from the OETH yields (which are distributed during rebases with increasing balances). Won't this forgo all unrealized yields as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like it has an internal log of oethCredits (updating during wrap and unwrap) but let me check

Copy link
Member

Choose a reason for hiding this comment

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

With WOETH ignore donations PR WOETH keeps the balance of the internal credits (of the OETH token) that change on mint, deposit, redeem, withdraw.

I think this approach is mathematically correct since the surplus figures out the difference between WOETH internal credits amount and OETH internal credits (for WOETH contract) multiplied by the credits per token - in other words operating with OETH token amounts.

Surplus could also be calculated using another approach by finding the difference of internal credits tracked by WOETH and OETH and then multiplying it by the current credits per token ( to arrive to the OETH token surplus amount).

I think in either of the approaches the un-realized rebase yields aren't left on the table, since as long as we do the correct math on the underlying credits per token. The rebase will take care of correct yield distribution.

Copy link
Member

Choose a reason for hiding this comment

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

Still a spot on comment @shahthepro 🙏 These are the sort of checks we need to make to be sure the math works correctly.

We could add a test for this @clement-ux, where:

  • WOETH has initial supply of 100 OETH
  • 2 OETH are transferred to the WOETH contract
  • OETH rebases (say increases balance of OETH on WOETH contract by x2)
  • governor should be able to transfer out 4 OETH and no more

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. Will check both PRs today to get more clarity on this.

Yeah, totally agree that we should be having unit and fork tests with different scenarios involving donations and rebases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the test that @sparrowDom suggested in this commit and it passes.

Copy link
Contributor Author

@clement-ux clement-ux Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Surplus could also be calculated using another approach by finding the difference of internal credits tracked by WOETH and OETH and then multiplying it by the current credits per token ( to arrive to the OETH token surplus amount).

Do you mean dividing instead of multiplying maybe?

An implementation could look something like this:

(uint256 creditsBalanceHighres, uint256 creditsPerTokenHighres, ) = OETH(asset()).creditsBalanceOfHighres(address(this));
uint256 surplus = (creditsBalanceHighres - oethCreditsHighres).divPrecisely(creditsPerTokenHighres);

This could consume less gas but can lead to rounding issues IMO. But as you already mentioned, I don't think it is important enough to increase complexity.

require(amount_ <= surplus, "Cannot collect OETH more than surplus");
clement-ux marked this conversation as resolved.
Show resolved Hide resolved
}

IERC20(asset_).safeTransfer(governor(), amount_);
}

Expand All @@ -95,7 +97,8 @@ contract WOETH is ERC4626, Governable, Initializable {
* @return amount of OETH credits the OETH amount corresponds to
*/
function _creditsPerAsset(uint256 oethAmount)
internal
internal
view
Copy link
Member

Choose a reason for hiding this comment

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

nice catch 👍

returns (uint256)
{
(, uint256 creditsPerTokenHighres, ) = OETH(asset())
Expand Down
38 changes: 34 additions & 4 deletions contracts/test/token/woeth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const { expect } = require("chai");
const { loadDefaultFixture } = require("../_fixture");
const { oethUnits, daiUnits, isFork } = require("../helpers");
const { hardhatSetBalance } = require("../_fund");
const { impersonateAndFund } = require("../../utils/signers");

describe("WOETH", function () {
if (isFork) {
Expand Down Expand Up @@ -105,7 +106,18 @@ describe("WOETH", function () {
await expect(woeth).to.have.approxBalanceOf("150", oeth);
await expect(woeth).to.have.a.totalSupply("50");
});

it("should decrease with an OETH negative rebase", async () => {
clement-ux marked this conversation as resolved.
Show resolved Hide resolved
await expect(woeth).to.have.approxBalanceOf("100", oeth);
await expect(woeth).to.have.a.totalSupply("50");
await expect(oeth).to.have.a.totalSupply("400");
// simulate a negative rebase of 1/4 of the supply.
await oeth
.connect(await impersonateAndFund(oethVault.address))
.changeSupply(oethUnits("300"));
await expect(oeth).to.have.a.totalSupply("300");
await expect(woeth).to.have.a.totalSupply("50"); // same total supply
await expect(woeth).to.have.approxBalanceOf("75", oeth); // 25% less than before
});
it("should not increase exchange rate when OETH is transferred to the contract", async () => {
await expect(woeth).to.have.a.totalSupply("50");
await expect(woeth).to.have.approxBalanceOf("100", oeth);
Expand Down Expand Up @@ -144,10 +156,28 @@ describe("WOETH", function () {
await expect(woeth).to.have.a.balanceOf("0", dai);
await expect(governor).to.have.a.balanceOf("1002", dai);
});
it("should not allow a governor to collect OETH", async () => {
it("should allow a governor to collect less than OETH surplus", async () => {
await oeth.connect(josh).transfer(woeth.address, oethUnits("2"));
await expect(woeth).to.have.a.balanceOf("102", oeth);
await woeth.connect(governor).transferToken(oeth.address, oethUnits("1"));
await expect(woeth).to.have.a.balanceOf("101", oeth);
await expect(governor).to.have.a.balanceOf("1", oeth);
await expect(await woeth.totalAssets()).to.equal(oethUnits("100"));
});
it("should allow a governor to collect exact OETH surplus", async () => {
await oeth.connect(josh).transfer(woeth.address, oethUnits("2"));
await expect(woeth).to.have.a.balanceOf("102", oeth);
await woeth.connect(governor).transferToken(oeth.address, oethUnits("2"));
await expect(woeth).to.have.a.balanceOf("100", oeth);
await expect(governor).to.have.a.balanceOf("2", oeth);
await expect(await woeth.totalAssets()).to.equal(oethUnits("100"));
});
it("should not a allow governor to collect more than OETH surplus", async () => {
await oeth.connect(josh).transfer(woeth.address, oethUnits("2"));
await expect(woeth).to.have.a.balanceOf("102", oeth);
await expect(
woeth.connect(governor).transferToken(oeth.address, oethUnits("2"))
).to.be.revertedWith("Cannot collect OETH");
woeth.connect(governor).transferToken(oeth.address, oethUnits("3"))
).to.be.revertedWith("Cannot collect OETH more than surplus");
});
it("should not allow a non governor to recover tokens ", async () => {
await expect(
Expand Down
Loading