WOETH: Withdraw OETH in surplus. #2119

clement-ux · 2024-07-03T10:28:14Z

Allow the governor to withdraw the OETH that has been sent in surplus to the wrapper.

Code Change Checklist

To be completed before internal review begins:

The contract code is complete
Executable deployment file
Fork tests that test after the deployment file runs
Unit tests *if needed
The owner has done a full checklist review of the code + tests

Internal review:

Two approvals by internal reviewers

Deploy checklist

Two reviewers complete the following checklist:

- [ ] All deployed contracts are listed in the deploy PR's description
- [ ] Deployed contract's verified code (and all dependencies) match the code in master
- [ ] The transactions that interacted with the newly deployed contract match the deploy script.
- [ ] Governance proposal matches the deploy script
- [ ] Smoke tests pass after fork test execution of the governance proposal

github-actions · 2024-07-03T10:28:47Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 4a12161

codecov · 2024-07-03T10:36:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.78%. Comparing base (56d0b7d) to head (4a12161).

Additional details and impacted files

@@                       Coverage Diff                       @@
##           sparrowDom/woeth_hack_proof    #2119      +/-   ##
===============================================================
+ Coverage                        62.76%   62.78%   +0.02%     
===============================================================
  Files                               66       66              
  Lines                             3373     3375       +2     
  Branches                           657      873     +216     
===============================================================
+ Hits                              2117     2119       +2     
  Misses                            1253     1253              
  Partials                             3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

openzeppelin-code · 2024-07-03T10:45:19Z

WOETH: Withdraw OETH in surplus.

Generated at commit: 4a12161fc17ec2526251b09eec9dc4c43d147f23

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 3 0 18 42 66
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

shahthepro · 2024-07-04T22:11:59Z

contracts/contracts/token/WOETH.sol

-        // 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();


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?

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

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.

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

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.

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

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.

contracts/test/token/woeth.js

contracts/contracts/token/WOETH.sol

sparrowDom · 2024-07-04T22:35:36Z

contracts/contracts/token/WOETH.sol

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


nice catch 👍

sparrowDom

Left a couple of comments, some minor changes and this PR is good to go.

Great job with it @clement-ux 🙏

sparrowDom

LGTM

clement-ux added 2 commits July 3, 2024 10:42

feat: withdraw surplus of oETH in wrapper.

7f6795a

test: add tests for surplus withdrawal.

4b10871

clement-ux added 4 commits July 3, 2024 14:16

fix: add visibility to internal function.

c3e83c4

test: add test for rebase with loss.

429cb14

style: run prettier.

21ea003

style: remove comment.

df78c6d

clement-ux marked this pull request as ready for review July 4, 2024 20:55

clement-ux requested review from sparrowDom, DanielVF, naddison36 and shahthepro as code owners July 4, 2024 20:55

shahthepro reviewed Jul 4, 2024

View reviewed changes

sparrowDom reviewed Jul 4, 2024

View reviewed changes

contracts/test/token/woeth.js Outdated Show resolved Hide resolved

sparrowDom reviewed Jul 4, 2024

View reviewed changes

contracts/contracts/token/WOETH.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Jul 4, 2024

View reviewed changes

clement-ux added 5 commits July 5, 2024 07:58

perf: reduce require string lenght.

9325358

fix: apply change on reverting string message to test.

09d4a44

test: cannot transfer more than surplus after rebase.

e192d51

style: run prettier.

afdccc1

test: remove test for negativ rebase.

4a12161

sparrowDom approved these changes Jul 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WOETH: Withdraw OETH in surplus. #2119

WOETH: Withdraw OETH in surplus. #2119

clement-ux commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading

openzeppelin-code bot commented Jul 3, 2024 •

edited

Loading

shahthepro Jul 4, 2024

shahthepro Jul 4, 2024

sparrowDom Jul 4, 2024

sparrowDom Jul 4, 2024

shahthepro Jul 5, 2024

clement-ux Jul 5, 2024

clement-ux Jul 5, 2024 •

edited

Loading

sparrowDom Jul 4, 2024

sparrowDom left a comment

sparrowDom left a comment

WOETH: Withdraw OETH in surplus. #2119

Are you sure you want to change the base?

WOETH: Withdraw OETH in surplus. #2119

Conversation

clement-ux commented Jul 3, 2024 • edited Loading

Code Change Checklist

Deploy checklist

github-actions bot commented Jul 3, 2024 • edited Loading

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

openzeppelin-code bot commented Jul 3, 2024 • edited Loading

WOETH: Withdraw OETH in surplus.

🚨 Report Summary

shahthepro Jul 4, 2024

Choose a reason for hiding this comment

shahthepro Jul 4, 2024

Choose a reason for hiding this comment

sparrowDom Jul 4, 2024

Choose a reason for hiding this comment

sparrowDom Jul 4, 2024

Choose a reason for hiding this comment

shahthepro Jul 5, 2024

Choose a reason for hiding this comment

clement-ux Jul 5, 2024

Choose a reason for hiding this comment

clement-ux Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

sparrowDom Jul 4, 2024

Choose a reason for hiding this comment

sparrowDom left a comment

Choose a reason for hiding this comment

sparrowDom left a comment

Choose a reason for hiding this comment

clement-ux commented Jul 3, 2024 •

edited

Loading

github-actions bot commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading

openzeppelin-code bot commented Jul 3, 2024 •

edited

Loading

clement-ux Jul 5, 2024 •

edited

Loading