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 0a988d5

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

DanielVF · 2024-06-27T19:30:50Z

contracts/contracts/token/WOETH.sol

+    using StableMath for uint256;
+    // doesn't need to be public, but convenient to be able to confirm the state on the mainnet
+    uint256 public oethCreditsHighres;
+    bool _oethCreditsInitialized;


Missing an explicit visibility.

thanks nice catch, corrected

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?

DanielVF · 2024-06-27T23:36:16Z

I misread the code and got the credits per token and the credits switched. Nevermind everything I said - I'll have to stare at it more!

contracts/contracts/token/WOETH.sol

DanielVF · 2024-06-27T23:56:44Z

contracts/contracts/token/WOETH.sol

+    function totalAssets() public view virtual override returns (uint256) {
+        (, uint256 creditsPerTokenHighres, ) = OETH(asset())
+            .creditsBalanceOfHighres(address(this));
+


If we used in credits here as well from the call from creditsBalanceOfHighres, we would have ever piece of data already loaded inside this function to check that our actual assets was equal or greater than our expected assets.

May or may not be such a great idea though to revert in a view function.

Hmm, that would be a good check that would detect possible mathematical irregularities (perhaps due to rounding). Though as you point out it would be weird to revert in a view function.

Monitoring sounds like a good place for it though. Have Grafana regularly query both credits values and check that the ones in OETH.sol are greater or equal to the ones in WOETH.sol.

Notion task added here: https://www.notion.so/originprotocol/WOETH-monitoring-sanity-check-695c73c0d0d04ef999a5242407d8a212

DanielVF · 2024-06-28T15:34:05Z

Invariants I can think of:

Assuming credits creditsPerTokenHighres in OETH only moves down, then woeth.convertToAssets() should only ever move up
We should always be able to withdraw all WOETH to OETH. (This is harder to test, so another way of phrasing this is that we should always have enough OETH on hand for everyone to withdraw)
OETH Donations to wOETH should not affect the convertToAssets() result
WETH donations to oeth can affect the convertToAssets() result upwards after a rebase, and that's fine.

contracts/contracts/token/WOETH.sol

openzeppelin-code · 2024-07-01T21:16:05Z

WOETH: Ignore donations

Generated at commit: 56d0b7d9eefc22b48877b8b980db284e6e41313e

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 3 0 18 43 67
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

contracts/contracts/token/WOETH.sol

pandadefi · 2024-07-10T13:44:31Z

A donation to increase the OETH price remains possible via the OETH contract but will require significant funds to increase the entire OETH vault value.

…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]>

shahthepro · 2024-12-17T12:52:56Z

contracts/contracts/token/WOETH.sol

+    using StableMath for uint256;
+    uint256 public oethCreditsHighres;
+    bool private _oethCreditsInitialized;


WOETHBase inherits WOETH, so might as well add some gaps here now

good point, done

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"?

shahthepro · 2024-12-17T12:55:26Z

contracts/contracts/token/WOETH.sol

+         * is internally being done in OETH:
+         */
+        // solhint-disable-next-line max-line-length
+        /** https://github.com/OriginProtocol/origin-dollar/blob/2314cccf2933f5c1f76a6549c1f5c9cc935b6f05/contracts/contracts/token/OUSD.sol#L242-L249


nit: Points to older code (pre-yield-delegation). Would be nice to update it to point to latest commit hash on master

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

minor test enhancement

ac855d9

DanielVF reviewed Jun 27, 2024

View reviewed changes

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

DanielVF reviewed Jun 27, 2024

View reviewed changes

DanielVF changed the title ~~WOETH lending markets adjustements~~ WOETH: Ignore donations Jun 28, 2024

DanielVF reviewed Jun 28, 2024

View reviewed changes

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

sparrowDom added 2 commits July 1, 2024 22:51

better function name

1c9fc64

remove comment

56d0b7d

shahthepro reviewed Jul 5, 2024

View reviewed changes

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

shahthepro reviewed Jul 5, 2024

View reviewed changes

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

shahthepro reviewed Jul 5, 2024

View reviewed changes

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

shahthepro reviewed Jul 5, 2024

View reviewed changes

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

pandadefi reviewed Jul 10, 2024

View reviewed changes

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

simplify

057469c

sparrowDom requested a review from clement-ux as a code owner December 17, 2024 10:14

sparrowDom added 3 commits December 17, 2024 11:35

simplify code

1a6a16f

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

add another test and simplify constructor

0a988d5

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 Jun 27, 2024

sparrowDom Jun 27, 2024

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

DanielVF Jun 27, 2024 •

edited

Loading

sparrowDom Jul 1, 2024

sparrowDom Jul 1, 2024

DanielVF commented Jun 28, 2024

openzeppelin-code bot commented Jul 1, 2024

pandadefi commented Jul 10, 2024

sparrowDom commented Dec 17, 2024

shahthepro Dec 17, 2024

sparrowDom Jan 6, 2025

shahthepro Dec 17, 2024

shahthepro Dec 17, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

DanielVF commented Jun 27, 2024

DanielVF Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVF commented Jun 28, 2024

openzeppelin-code bot commented Jul 1, 2024

WOETH: Ignore donations

🚨 Report Summary

pandadefi commented Jul 10, 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

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 Jun 27, 2024 •

edited

Loading