WOETH: Ignore donations #2106

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

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 56d0b7d

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

Codecov Report

Attention: Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.76%. Comparing base (c9f069e) to head (56d0b7d).
Report is 5 commits behind head on master.

Files	Patch %	Lines
contracts/contracts/token/WOETH.sol	96.07%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2106      +/-   ##
==========================================
+ Coverage   61.86%   62.76%   +0.90%     
==========================================
  Files          66       66              
  Lines        3322     3373      +51     
  Branches      863      657     -206     
==========================================
+ Hits         2055     2117      +62     
+ Misses       1264     1253      -11     
  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?

sparrowDom · 2024-06-27T20:30:21Z

@DanielVF not completely sure what you mean. So the logic is that:

when the contract is initialized (on proposal execution) the internal credits of OETH are read and stored in WOETH. From that point forward the WOETH doesn't ask for internal credits OETH contract is holding but rather keeps its own accounting
on each mint, deposit, withdraw, redeem the current credits per token are fetched from OETH (since we don't know if a rebase happened since the last time we queried it) and the accounting of credits in WOETH contracts are updated. Since only those 4 functions trigger the update of credits a normal transfer of OETH to WOETH contract has no effect. There is also a test for this where you can see that after OETH transfer the WOETH contract will still have a balance of 50 OETH and totalSupply & totalAssets of 0.

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

shahthepro · 2024-07-05T07:20:51Z

contracts/contracts/token/WOETH.sol

+        if (_oethCreditsInitialized) {
+            require(false, "Initialize2 already called");
+        }


Why not just require(!_oethCreditsInitialized, "Initialize2 already called");?

shahthepro · 2024-07-05T07:24:24Z

contracts/contracts/token/WOETH.sol

+     * @param oethAmount Amount of OETH to be converted to OETH credits
+     * @return amount of OETH credits the OETH amount corresponds to
+     */
+    function _creditsPerAsset(uint256 oethAmount)


nit: This function name confuses me a bit. When I see per asset, I assume it's gonna return credits for 1 OETH. But it takes in a oethAmount and multiplies the credits by it. So, it sounds more like creditsForAssetAmount than creditsPerAsset

shahthepro · 2024-07-05T07:34:15Z

contracts/contracts/token/WOETH.sol

+        require(
+            assets <= maxDeposit(receiver),
+            "ERC4626: deposit more then max"
+        );
+
+        address caller = _msgSender();
+        uint256 shares = previewDeposit(assets);
+
+        // if _asset is ERC777, transferFrom can call reenter BEFORE the transfer happens through
+        // the tokensToSend hook, so we need to transfer before we mint to keep the invariants.
+        SafeERC20.safeTransferFrom(
+            IERC20(asset()),
+            caller,
+            address(this),
+            assets
+        );
+        _mint(receiver, shares);
+        oethCreditsHighres += _creditsPerAsset(assets);
+
+        emit Deposit(caller, receiver, assets, shares);
+
+        return shares;


Could this be simplified into just (to avoid code-duplication):

shares = super.deposit(assets, receiver); oethCreditsHighres += _creditsPerAsset(assets);

Same with other overridden functions as well

shahthepro · 2024-07-05T07:36:32Z

contracts/contracts/token/WOETH.sol

+        _burn(owner, shares);
+        oethCreditsHighres -= _creditsPerAsset(assets);


Haven't tested it yet but I wonder if we should compute the credits before burn? Let me see if it makes any difference or if it's breakable

I tried running some tests. Change in order didn't affect the numbers at all

pandadefi · 2024-07-10T12:51:02Z

contracts/contracts/token/WOETH.sol

+    }
+
+    /** @dev See {IERC4262-mint} */
+    function mint(uint256 shares, address receiver)


You could save on some code complexity calling deposit from the mint function to prevent having most of the same code twice.

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.

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

add explicit visibility

6822937

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

pandadefi reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WOETH: Ignore donations #2106

WOETH: Ignore donations #2106

sparrowDom commented Jun 21, 2024 •

edited by clement-ux

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

sparrowDom commented Jun 27, 2024 •

edited

Loading

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

shahthepro Jul 5, 2024

shahthepro Jul 5, 2024

shahthepro Jul 5, 2024

shahthepro Jul 5, 2024

shahthepro Jul 5, 2024

shahthepro Jul 5, 2024

pandadefi Jul 10, 2024

pandadefi commented Jul 10, 2024

		_burn(owner, shares);
		oethCreditsHighres -= _creditsPerAsset(assets);

WOETH: Ignore donations #2106

Are you sure you want to change the base?

WOETH: Ignore donations #2106

Conversation

sparrowDom commented Jun 21, 2024 • edited by clement-ux Loading

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

sparrowDom commented Jun 27, 2024 • edited Loading

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

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

pandadefi commented Jul 10, 2024

sparrowDom commented Jun 21, 2024 •

edited by clement-ux

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

sparrowDom commented Jun 27, 2024 •

edited

Loading

DanielVF Jun 27, 2024 •

edited

Loading