Simplified donation attack prevention

[sparrowDom]

We are planning to prevent big yield jumps by modifying code directly in VaultCore in this PR: #2452
This allows for simplification in WOETH to completely rely on rebasingCreditsPerToken to deduce its own exchange rate.

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

[sparrowDom]

Sonic deploy and basic tests along with cotract extending WOETH done here: 191f522

[DanielVF]

⚪️ Lol, let's change both of these to the correct IERC4626

[sparrowDom]

changed it to the natspec compliant @inheritdoc: a2a69c9

[sparrowDom]

Another valid approach here would be that in case of 0 totalSupply we could set the adjuster to rebasingCredits:

_adjuster = rebasingCreditsPerTokenHighres();

The difference in behaviour occurs if we deposit a fresh wrapped contract after rebases on the underlying token have already occurred. In case we set the adjuster to rebasingCreditsPerTokenHighres() the WOETH -> OETH exchange rate would start at 1:1 and follow the underlying token's rebase rate.
If we keep it at 1e27 the exchange rate will follow the value accrual of OETH from inception.

[DanielVF]

I think I prefer the 1e27 approach - bigger number is more accuracy? Plus we'll be deploying this soon after launch on new tokens, so the end number should be very similar.

[DanielVF]

WOETH Audit

Requirements

We want to prevent donation attacks against lending protocols using our system. We want to prevent someone donating to this contract to instantly increase the exchange rate.

In this code, the exchange rates are directly tied to the OETH exchange rates without any consideration for the shares or assets on WOETH. Once initialized, there is no way to affect the exchange rate on this contract without going through the vault (which now has limits). This should meet our needs.

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

No crypto

Gas problems

No for loops

Black magic

No magic

Overflow

• Code is solidity version >= 0.8.0

Proxy

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

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
  • 🟢 TotalAssets rounds down, which underestimates actual assets.
• Contract does not have bugs from loosing rounding precision
  • 🟢 Have fork tested stupidly large OETH rebase ratios with precision loss, and WOETH errors in the correct direction.
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
  • 🟢 When adjuster is zero, mints/deposits are disabled because of a divide by zero error. On new contracts there can be no supply until after adjuster is set.
  • 🟢 Adjuster cannot be changed back to zero once it has a positive number, because of the require on the init.
  • 🟣 If you were to migrate (initialize2) a contract with a very small totalSupply (say 1), and have donated some assets to it, you could force the contract into having big rounding errors. Ajuster errors favor deposits. Something like the classic first donation attack on 4626 contracts. However, we only have a few old wrapped tokens, and they already have lots of funds in them and will have sane adjustor values. New deploys are immune since they will go to a hardcoded 1e27.
  • zero totalSupply() or zero totalAssets() does not affect exchange rates in or out.
• Safecast is aways used when casting

Dependencies

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

External calls

• Contract addresses passed in are validated
  • 🟢 No user controlled addresses
• No unsafe external calls
• Reentrancy safe
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Deploy

• Deployer permissions are removed after deploy

Thinking

Logic

Good, see notes under flavor.

Deployment Considerations

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

Internal State

We want all shares to be redeemable. The worst case for this is a single withdraw of all shares, which is represented by totalAssets(). Therefor totalAssets() should return a number that is less than the actual assets held by the contract. This has been the case in all my testing. Rounding errors on deposits and withdraws add to the solvency, it is not given to users, and rebasing also rebases the solvency dust.

Attack

These wrapped token contracts usually contain a large amount of funds - up to half the tokens for a given Origin product may be in the wrapped contract. If an attacker found a way to drain these funds, they would be very happy.

🟢 The easiest way to drain the assets would be to have a time when convert shares and convert assets cross each other. Because convertAssets() and convertShares() are computed from the same numbers, just with different rounding directions, they should never cross.

🟢 Alternately, you can find a way to get convert shares and convert assets to cross each other over time. Obviously they cross each other in a way as yield goes up, but that’s just earning yield, and does not negatively affect other depositors. In theory the rounding error in deposit changes from equal to unfavorable. However this should not let an attacker beat the ratio.

😜 Once rounding errors became a thing, an attacker could front run deposits with tiny deposits of their own, maximizing depositor rounding error. The attacker wouldn’t even profit if holding OETH, since all rounding error is by design stored by the contract and not shared with users.

🟢 If the exchange rate could be manipulated rapidly either up or down, it could cause problems for AMMs or lending platforms using this. In theory, the exchange rate cannot be manipulated down because OE's exchange rate cannot go down. Upwards should be controlled by the vault, and rate limited.

🟢 This code is sensitive to the exact nuances of how OUSD rebases and may have tiny insolvency issues if there is some mismatch in behavior. I’ve fork tested that with ludicrous totalSupply growth, the contract gets more solvent, not less.

🟢 Trappped dust will continue to stay in the contract and grow. Should be fine though.

🟢 There is an instant update of the exchange rate when the exchange rate of OUSD changes. However, this is actually a more restrictive situation than it used to be - before the exchange rate would instantly update on OUSD exchange rate changes as well as on donations or changes in totalAssets()/totalSupply(). At a minimum we are no worse than before.

Flavor

Probably not worth making the below changes after so much security review has happened, but I think it would be more beautiful to:

• use convertToAssets() in totalAssets(), rather than duping the calculation
• and change rebasingCreditsPerTokenHighres() to _rebasingCreditsPerTokenHighres()

[sparrowDom]

Requirements

The PR fixes a problem with our Wrapped OETH contract that was vulnerable to donation attacks. It also uses a unique way of using OETH's rebasingCreditsPerToken to trim down the math required to convert between assets and shares.

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

Proxy

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

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
  • convertToAssets will always round down. Meaning for a given amount of shares it will round the asset decimals down - like Math.floor(). Consequences:
    • maxWithdraw will report a rounded down amount of assets a user can withdraw - favouring the protocol
    • previewMint rounds up becase of convertToShares(convertToAssets(shares)) adding 1 wei when needed
    • previewRedeem will round down the amount of assets the user receives when redeeming shares.
    • 🟡 for all this to be safe the convertToShares(convertToAssets(shares)) must never return more than 1 wei difference in shares to the original shares passed to the convertToAssets. Basic fuzz testing confirmed this behavioue
  • convertToShares
    • previewDeposit rounds down when transforming assets to shares
    • is used in previewMint & previewWithdraw for the purposes of rounding up
  • most importantly non of the above functions include the contract's total supply, to it is immune to weird behaviour at specific totalSupply numbers.
• 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
  • now that the contract doesn't use totalSupply in convertToShares & convertToAssets it should not misbehave when totalSupply is near zero
• Safecast is aways used when casting. no safecast

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
  • OZ has done immense changes to 4626, but we've decided to not include them since precision math issues have been customely addressed by converting between assets and shares solely using OETH's rebasingCreditsPerToken
• 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

• Contract addresses passed in are validated
• No unsafe external calls
  • we only call OETH and that is a trusted contract
• Reentrancy guards on all state changing functions
  • no need as we only call the OETH trusted contarct
• No malicious behaviors
• Oracles
  • reading rebasingCreditsPerToken from OETH and that one can not be manipulated in malicious manner
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Remove this section if the code being reviewed is not a strategy.

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Logic is really trimmed down to the bare necessities. Not using supply and totalAssets when converting between shares and assets makes logic very clean and robust.

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

Deployment Considerations

The contract handles fresh WOETH deployment and existing one correctly by setting the right adjuster.

Internal State

There isn't much internal state to this contract. All it tracks is its totalSupply but importantly doesn't use it when doing calculation between assets and shares. There is an adjuster variable that gets set on deployment and that can't be manupulated. The important rebasingCreditsPerTokenHighres() is always being queried realtime from a trusted OETH contract.

Attack

A user somehow manipulating the conversion between shares and assets in such a way that it would cause rounding issues. Analysis of all possible paths hasn't revealed such possibility

Flavor

Code is very clean and probably can not reach further simplification.

[shahthepro]

Requirements

Fix the logic to prevent donation attacks on wOETH contract.

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:
  • A way to remove items
  • Can be upgraded to get unstuck
  • 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.

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

Proxy

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

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• 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
• Safecast is aways used when casting

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• 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

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

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.

Deploy

• Deployer permissions are removed after deploy

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

• Correct usage of global & local variables

Deployment Considerations

N/A

Internal State

adjuster is the only new variable added to the contract. Ideally it could have been a immutable set in the constructor. However, it relies on OETH's state and a few local functions (totalSupply), which might not be available in the implementation contract.

Attack

Couldn't think of anything. Simulated a few edge case scenarios and they all worked as expected.

Flavor

The code is simple and easy to understand.

🔴 The code inherits from another older PR, but almost all of the changes from that PR are removed in this branch. Would be great if we can point this to master instead of the older PR. The code is not synced with master for over a month, it may lead to merge conflicts and unnecessary behaviour of contracts/tests as it is.

🟡 The code only updates the wOETH contract. Doesn't change wOUSD and wOETHb tokens.

The base branch was changed.

[sparrowDom]

thanks @shahthepro all issues addressed:

• PR branched off of mainnet
• WrappedOUSD now extends WOETH
• added deploy file and test files for WOUSD