WOETH: Donation attack prevention

[sparrowDom]

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:

• [x] The contract code is complete
• [x] Executable deployment file
• [x] Fork tests that test after the deployment file runs (done with a brownie script to confirm that existing WOETH balances are not affected)
• [x] Unit tests *if needed
• [x] The owner has done a full checklist review of the code + tests

Internal review:

• [x] Two approvals by internal reviewers

[github-actions[bot]]

	Warnings
:warning:	:eyes: This PR needs at least 2 reviewers

Generated by :no_entry_sign: dangerJS against 79af39f4cc3b4d06c6fa58c3115aee9678b1649a

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[sparrowDom]

Requirements

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

Easy Checks

Authentication

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

Ethereum

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

Cryptographic code

• [x] This contract code does not roll it's own crypto.
• [x] No signature checks without reverting on a 0x00 result.
• [x] 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

• [x] Does not contain selfdestruct
• [x] 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.
• [x] Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

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

Proxy

• [x] No storage variable initialized at definition when contract used as a proxy implementation.

Events

• [x] All state changing functions emit events

Medium Checks

Rounding

• [x] 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
• [x] Contract does not have bugs from loosing rounding precision
• [x] Code correctly multiplies before division
• [x] 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.~~
• [x] Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• [x] Contract addresses passed in are validated
• [x] No unsafe external calls
• [ ] ~~Reentrancy guards on all state changing functions~~no need we only deal with our own OETH - trusted contract
  • [ ] ~~Still doesn't protect against external contracts changing the state of the world if they are called~~.
• [x] No malicious behaviors
• [x] Low level call() must require success.
• [x] No slippage attacks (we need to validate expected tokens received)
• [x] Oracles, one of:
  • [x] No oracles
  • [ ] Oracles can't be bent
  • [ ] If oracle can be bent, it won't hurt us.
• [x] Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• [x] Each publicly callable method has a test
• [x] Each logical branch has a test
• [x] Each require() has a test
• [x] 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

• [x] Deployer permissions are removed after deploy

Thinking

Logic

Are there bugs in the logic?

• [x] 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

[DanielVF]

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]

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

[sparrowDom]

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

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!

[DanielVF]

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.

[openzeppelin-code[bot]]

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

[pandadefi]

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]

@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

[DanielVF]

🔴 Requirements

We want to make our non-rebasing wrapper contracts able to be safe to be borrowed on lending platforms, not just used as collateral.

There’s a little known attack in DeFi, where a wrapper token that can be donated to can be used in an attack. The details of this attack we will skip here, but it requires:

1. Wrapper token that can be instantly redeemed for its base asset
2. Wrapper token can have it’s exchange rate go up instantly from donations.
3. Oracles that instantly update the true value of that wrapper token.
4. There's not a lot of TVL in the wrapper yet, relative to lending platform configuration for the asset.
5. That wrapper token can have unlimited borrowing.

My concern is that other coins who try to stop this attack, do so by slowly dripping out yield on their wrapped tokens. This means that flash loans are completely ineffective and an alternative slow approach allows a lending platforms to profitably liquidate anyone trying it.

This PR works by completely blocking donations to the wrapper contract, but donations to vault are passed through instantly. This still dilutes an attack, but only by the difference between the vault and wrapper token. This PR still increases the attack cost 2x-3x on our current coins, instead of the 7,000x cost increase with the yield drip approach.

Now a 2x-3x difficulty increase is certainly nice, and we get to a safe TVL size sooner, but it means that we still need to do the math on any particular lending platform to see if okay to be borrowed.

	Circ in wrapper	Wrapped Value	Protocol Value	10% Attack Before	10% Attack After
OUSD	1.12%	100,762	9,010,263	10,076	901,026
OETH	43.15%	39,856,238	92,357,298	3,985,624	9,235,730
SuperOETHb	30.87%	13,891,111	45,000,000	1,389,111	4,500,000
OS	43.68%	11,260,398	25,780,243	1,126,040	2,578,024

I’m quite concerned that without the time drip, we only have a small band-aid, rather than a strong block.

[sparrowDom]

Some additional info on the viability of inflation/donation attack. (for reference see xSushi Aave vulnerability incident report)

How the attack works

Before we are able to evaluate how much security a 2-3x increase in difficulty provides we should understand how the attack works:

1. The attacker has 2 account A & B
2. Account A takes a big Flashloan say 1b in ETH
3. takes that eth and supplies it to a lending platform.
4. Account A borrows as much WOETH as possible and sends it to account B
5. Account B supplies that WOETH to the lending platform

Steps 4 & 5 are repeated as long as the Loan To Value amount on the lending platform allows it. Say it was possible to loop steps so many times that Account A owes 500m in WOETH and Account B has supplied all WOETH to the lending platform. To sum up:

Account A:

• has borrowed 500m WOETH
• has supplied 1b of ETH

Account B:

• has supplied 500m WOETH

6. Now the attacker send OETH to the WOETH contract, or WETH to the OETH Vault to inflate the exchange rate between OETH & WOETH. Since OETH TVL is 92m the attacker spends 184m to inflate the price of WOETH by 200% making 1 WOETH worth 3 OETH.
7. Now the value of Account A's liabilities (WOETH) is 1.5b, while the tokens backing are only worth 1b. Account A is liquidated
8. Account B now has 1.5b worth of collateral (in price inflated WOETH). It can borrow ETH to pay back the 500m flash loan, and 184m of OETH price increase. Everything else - 816m is profit. That is used to borrow any available assets on the lending platform.

Observations

• looping of the wrapped token from account A to B is what multiplies the amount invested into donation attack to gained profits
• the lending platform needs to have enough assets to borrow available to cover the expense of the donation attack and the initial flash loan
• If a Wrapped token can be attacked directly it is easier to exploit. As the attack is much cheaper in that case: in the last step of looping wrapped tokens between A & B the account A can unwrap all of the wrapped token to minimise its TVL. This means that a direct donation to the wrapped token will be a lot cheaper.
• Great props to @pandadefi and @DanielVF for pointing out that the donation attack on the Vault can still put lending platfroms at risk.