WOETH: Donation attack prevention #2106

sparrowDom · 2024-06-21T20:44:39Z

Overview

This PR prevents an attacker to manipulate the exchange rate between WOETH & OETH by donating OETH to the contract .

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 (done with a brownie script to confirm that existing WOETH balances are not affected)
Unit tests *if needed
The owner has done a full checklist review of the code + tests

Internal review:

Two approvals by internal reviewers

github-actions · 2024-06-21T20:45:11Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against aad7b57

codecov · 2024-06-21T20:52:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.85%. Comparing base (fa077cd) to head (f0580bc).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2106      +/-   ##
==========================================
+ Coverage   53.26%   53.85%   +0.58%     
==========================================
  Files          79       79              
  Lines        4098     4120      +22     
  Branches     1079     1081       +2     
==========================================
+ Hits         2183     2219      +36     
+ Misses       1912     1898      -14     
  Partials        3        3

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

sparrowDom · 2024-06-26T15:03:08Z

Requirements

What is the PR trying to do? Is this the right thing? Are there bugs in the requirements?
No

Easy Checks

Authentication

Never use tx.origin
Every external/public function is supposed to be externally accessible
Every external/public function has the correct authentication

Ethereum

Contract does not send or receive Ethereum.
Contract has no payable methods.
Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

This contract code does not roll it's own crypto.
No signature checks without reverting on a 0x00 result.
No signed data could be used in a replay attack, on our contract or others.

Gas problems

~~Contracts with for loops must have either~~: no loops
- ~~A way to remove items~~ no loops
- ~~Can be upgraded to get unstuck~~ no loops
- ~~Size can only controlled by admins~~ no loops
~~Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.~~ no loops

Black magic

Does not contain selfdestruct
Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

Code is solidity version >= 0.8.0
~~[ ] All for loops use uint256~~ no loops

Proxy

No storage variable initialized at definition when contract used as a proxy implementation.

Events

All state changing functions emit events

Medium Checks

Rounding

Contract rounds in the protocols favor it doesn't but it mimics what OETH is doing using mulTruncateon high resolution credits and high resolution credits per token. That should ensure the correct rounding of the values
Contract does not have bugs from loosing rounding precision
Code correctly multiplies before division
Contract does not have bugs from zero or near zero amounts

Dependencies

~~[ ] Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.~~ no new dependancies added
~~If OpenZeppelin ACL roles are use review & enumerate all of them.~~
Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

Tests

Each publicly callable method has a test
Each logical branch has a test
Each require() has a test
Edge conditions are tested
~~If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.~~ _ it doesn't_

Deploy

Deployer permissions are removed after deploy

Thinking

Logic

Are there bugs in the logic?

Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

Are there things that must be done on deploy, or in the wider ecosystem for this code to work. Are they done?
Nothing special needs to happen on deploy

Internal State

What can be always said about relationships between stored state
What must hold true about state before a function can run correctly (preconditions)
What must hold true about the return or any changes to state after a function has run.

For all 3 questions above it is important that: The internal credits stored in WOETH and stored in OETH (for WOETH contract) should always match unless someone sends extra OETH to the WOETH contract manually.

Does this code do that?
Yes

Attack

What could the impacts of code failure in this code be.
Incorrect OETH balance tracking within WOETH contract. Which would result in incorrect pricing of WOETH.

What conditions could cause this code to fail if they were not true.
Math tracking credits within WOETH differentiating from the one in OETH.

Does this code successfully block all attacks.
yes

Flavor

Could this code be simpler?
No
Could this code be less vulnerable to other code behaving weirdly?
No

…ck_proof

contracts/contracts/token/WOETH.sol

DanielVF · 2024-06-27T20:01:47Z

The core attack we are trying to stop is someone sending the OETH to the wOETH contract, causing the value of wOETH in OETH terms to go suddenly up.

It looks like totalAssets uses the amount of OETH held by the contract as one of two multipliers. totalAssets is in turn used to calculate the exchange ratio. If someone donates to the contract, one of these two multipliers goes up, and the donation has perfectly succeeded in increasing the value of each wOETH. This attack does not appear to be blocked at all?

Or am I missing something?

DanielVF · 2024-06-27T20:04:18Z

It also feels really scary that were are minting and burning using old ratios. That doesn't cause rektness?

…ck_proof

sparrowDom · 2024-12-17T11:47:25Z

@pandadefi yes the donation attack on OETH with donating WETH to the Vault and calling rebase is ok. This is the way OETH increases its value and I can't think of a way how it could be exploited

* Add wOETH donation fork tests * test: add fork test for deposit/mint/withdraw/redeem. * test: add test for redeem after rebase. --------- Co-authored-by: clement-ux <[email protected]>

contracts/contracts/token/WOETH.sol

shahthepro · 2024-12-17T12:54:15Z

contracts/contracts/token/WOETH.sol

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


nit: perhaps we could just treat any donation as "yield"?

While we shouldn't treat donations as instant yield (that's what this contract is trying to get away from), I do think we should build in a separate governor method to collect donated funds that are in excess of the backing funds.

We should probably merge this PR in? #2119

contracts/contracts/token/WOETH.sol

DanielVF · 2025-02-20T18:01:18Z

contracts/contracts/token/WOETH.sol

@@ -31,11 +43,40 @@ contract WOETH is ERC4626, Governable, Initializable {
        OETH(address(asset())).rebaseOptIn();
    }

-    function name() public view virtual override returns (string memory) {
+    function initialize2() external onlyGovernor {


Perhaps we should have initialize() call initialize2(). This way new contract deploys don't need to call both, and we are less likely to make the bad mistake of not calling initialize2().

Great idea thanks: 5a7192d

contracts/contracts/token/WOETH.sol

DanielVF · 2025-02-20T18:11:59Z

contracts/contracts/token/WOETH.sol

+        uint256 woethAmount,
+        address receiver,
+        address owner
+    ) public virtual override returns (uint256 oethAmount) {


This contract does not currently compile. This should be fixed.

Also, although this is not the actual compile error, I'm wondering if these virtuals in these methods are wrong, since I think we want these functions callable without needing to be overridden by a child class.

Yes you are right these virtual keywords are not needed. We can add it in the future if need be: 5ff5c5d

DanielVF · 2025-02-20T18:27:25Z

contracts/contracts/token/WOETH.sol

+     * @return amount of OETH credits the OETH amount corresponds to
+     */
+    function _oethToCredits(uint256 oethAmount) internal returns (uint256) {
+        (, uint256 creditsPerTokenHighres, ) = OETH(asset())


Both _oethToCredits() and totalAssets() call oeth.creditsBalanceOfHighres() to get the creditsPerToken, discarding the other values that function returns.

I think it makes more sense to call the simpler oeth.rebasingCreditsPerTokenHighres() instead.

There's two scenarios here:

This wrapped token is correctly marked as rebasing. In this case, oeth.rebasingCreditsPerTokenHighres() will return the same value as what we are doing now, but be simpler, return only what we need, and cost less gas.

Governance messes up the world in a bad way and turns off yield to the contract. The current call will immediately return 1e18 instead of 1e27ish, making for a really really wrong totalAssets, and thus really really wrong conversion rate, which would roughly speaking destroy the wrapped token and anything else using it. However, if oeth.rebasingCreditsPerTokenHighres() is used we'll only get a gradual drift off the correct value as expected yield does not come in.

In both cases the behavior of oeth.rebasingCreditsPerTokenHighres() seems better.

Great comment and great points thanks: 3ef2219

DanielVF · 2025-02-20T18:30:55Z

contracts/contracts/token/WOETH.sol

+        address owner
+    ) public virtual override returns (uint256 oethAmount) {
+        oethAmount = super.redeem(woethAmount, receiver, owner);
+        oethCreditsHighres -= _oethToCredits(oethAmount);


This is just a mathematical nit, and perhaps the code is okay without this, but in general, if you are depositing/plus-ing and withdrawing/minus-ing using the same conversion function, you are almost certainly rounding the wrong direction in one of them. It's possible we need two _oethToCredits, one that round up, and one that rounds down.

good point I need to sleep on this one....

…ck_proof

This reverts commit 637cd3c.

DanielVF · 2025-02-25T19:07:55Z

contracts/contracts/token/WOETH.sol

+    }
+
+    /** @dev See {IERC4262-totalAssets} */
+    function totalAssets() public view override returns (uint256) {


🔴 rebasingCreditsPerToken has the property that the smaller it is, the more money a user has.

Default solidity division results in rounding down. Therefore this totalAssets() method is rounding up the amount of money in contract. Therefore it is very easy for the contract to be a rounding error on the wrong side of solvent.

That said, we should be careful to fuzz test the behavior differences. There's no perfect answer here, and so we want to make sure that erroring on the solvent side is correct.

This is all it takes for the contract to report insolvancy with the current code.

sparrowDom added 2 commits June 21, 2024 13:43

intermittent work committed

2839dc1

add draft solution for preparing WOETH for the landing markets

2e6be62

sparrowDom added 3 commits June 24, 2024 21:11

add tests that confirm lending market protection works as exepcted

5495d4b

add test to protect agains calling initialize twice

d0eb16f

add comments

fc5d874

sparrowDom marked this pull request as ready for review June 26, 2024 08:44

sparrowDom requested review from naddison36 and shahthepro as code owners June 26, 2024 08:44

sparrowDom added 9 commits June 26, 2024 10:45

remove unneeded files

cfec4e5

minor bug fix

8405bfc

add fork script tests and fix bug

e03223c

add comments

319467f

add another test

1142a74

divide after multiply

f911ba6

change credits per token to highres

3dc8523

add comment

bad282b

comments

07b51cf

sparrowDom added 3 commits June 26, 2024 17:05

lint

c0333fb

minor changes

2c965c3

Merge remote-tracking branch 'origin/master' into sparrowDom/woeth_ha…

df57ef7

…ck_proof

sparrowDom requested a review from DanielVF as a code owner June 26, 2024 15:45

sparrowDom added 3 commits June 26, 2024 18:52

correct fixture

13fd4da

make code slighlty better regarding re-entry

7923ddb

prettier

c69583f

DanielVF reviewed Jun 27, 2024

View reviewed changes

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

sparrowDom added 2 commits December 17, 2024 12:09

simplify code

2223600

Merge remote-tracking branch 'origin/master' into sparrowDom/woeth_ha…

b34d811

…ck_proof

sparrowDom added 3 commits December 17, 2024 13:22

refactor

39e8d4b

add redeem all test

3615ef7

prettier

466f957

sparrowDom changed the title ~~WOETH: Ignore donations~~ WOETH: Donation attack prevention Dec 17, 2024

shahthepro and others added 2 commits December 17, 2024 16:49

Add wOETH donation fork tests (#2122)

52eb244

* Add wOETH donation fork tests * test: add fork test for deposit/mint/withdraw/redeem. * test: add test for redeem after rebase. --------- Co-authored-by: clement-ux <[email protected]>

rename deploy script

f0580bc

shahthepro reviewed Dec 17, 2024

View reviewed changes

sparrowDom and others added 3 commits December 17, 2024 14:22

add another test and simplify constructor

0a988d5

fix: specify override for name and symbol.

e2eb8cf

fix: import IERC20Metadata.

c6ded55

DanielVF reviewed Feb 20, 2025

View reviewed changes

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

DanielVF reviewed Feb 20, 2025

View reviewed changes

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

DanielVF reviewed Feb 20, 2025

View reviewed changes

sparrowDom added 10 commits February 24, 2025 20:47

add gap

be6d473

update reference to code

c38a3c6

fix compile errors

66269c8

move initialize2() into primary initializer

5a7192d

remove virtual where not needed

5ff5c5d

Merge remote-tracking branch 'origin/master' into sparrowDom/woeth_ha…

a05eaa5

…ck_proof

pick a more appropriate method for fetching highres CPT

3ef2219

round in the favour of the protocol

637cd3c

Revert "round in the favour of the protocol"

390e4a2

This reverts commit 637cd3c.

rounding error fixes

aad7b57

DanielVF reviewed Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WOETH: Donation attack prevention #2106

WOETH: Donation attack prevention #2106

sparrowDom commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading

sparrowDom commented Jun 26, 2024 •

edited

Loading

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

sparrowDom commented Dec 17, 2024

shahthepro Dec 17, 2024

DanielVF Feb 20, 2025

DanielVF Feb 24, 2025

DanielVF Feb 20, 2025 •

edited

Loading

sparrowDom Feb 24, 2025

DanielVF Feb 20, 2025

sparrowDom Feb 24, 2025

DanielVF Feb 20, 2025 •

edited

Loading

sparrowDom Feb 24, 2025

DanielVF Feb 20, 2025

sparrowDom Feb 24, 2025

DanielVF Feb 25, 2025

DanielVF Feb 25, 2025

DanielVF Feb 25, 2025

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

WOETH: Donation attack prevention #2106

Are you sure you want to change the base?

WOETH: Donation attack prevention #2106

Conversation

sparrowDom commented Jun 21, 2024 • edited Loading

Overview

Code Change Checklist

github-actions bot commented Jun 21, 2024 • edited Loading

codecov bot commented Jun 21, 2024 • edited Loading

Codecov Report

sparrowDom commented Jun 26, 2024 • edited Loading

Requirements

Easy Checks

Authentication

Ethereum

Cryptographic code

Gas problems

Black magic

Overflow

Proxy

Events

Medium Checks

Rounding

Dependencies

External calls

Tests

Deploy

Thinking

Logic

Deployment Considerations

Internal State

Attack

Flavor

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

sparrowDom commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVF Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVF Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparrowDom commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading

sparrowDom commented Jun 26, 2024 •

edited

Loading

DanielVF Feb 20, 2025 •

edited

Loading

DanielVF Feb 20, 2025 •

edited

Loading