Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid zero value transfers #1014

Merged
merged 31 commits into from
Oct 1, 2024
Merged

Avoid zero value transfers #1014

merged 31 commits into from
Oct 1, 2024

Conversation

EndymionJkb
Copy link
Collaborator

Description

Per auditor comment, support tokens that prohibit zero transfers (and save a little gas, at the cost of a bit of bytecode), by checking for 0 amounts and refraining from token or ETH transfers of zero value.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Optimization: [ ] gas / [ ] bytecode
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • Complex code has been commented, including external interfaces
  • Tests have 100% code coverage
  • The base branch is either main, or there's a description of how to merge

Issue Resolution

Resolves #993.

@EndymionJkb
feat: block zero transfers for pool tokens (simulate tokens that don'…
6c5069c 
…t support this)
@EndymionJkb
refactor: check amount > 0 in _sendTo, to prevent zero transfers
90ff425
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
f0cf956
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
d5d0010
@EndymionJkb
feat: protect router against zero transfers
6f69649
@EndymionJkb
test: protect hooks against zero transfers
29b0de2
@EndymionJkb
chore: update bytecode
ed9c989
@EndymionJkb
chore: update gas
175b654
@EndymionJkb
feat: also prevent 0 ETH transfers
68b527a
@EndymionJkb
chore: update bytecode
a148d76
@EndymionJkb
chore: update gas
7858591
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
1d89cfc 
# Conflicts:
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] add liquidity using swapExactOur - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] swap exact in with one token and fees - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard] add liquidity proportional
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard] add liquidity single token exact out - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard] add liquidity unbalanced - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] add liquidity using swapExactOur - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] swap exact in with one token and fees - cold slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate] add liquidity proportional
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate] add liquidity single token exact out - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool] donation
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard - BatchRouter] add liquidity using swapExactOur - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard] add liquidity proportional
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - Standard] add liquidity single token exact out - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] add liquidity using swapExactOur - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] swap exact in with one token and fees - cold slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] swap exact in with one token and fees - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate] add liquidity proportional
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate] add liquidity single token exact out - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate] add liquidity unbalanced - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool] donation
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
08427db 
# Conflicts:
#	pkg/vault/test/.contract-sizes/BatchRouter
#	pkg/vault/test/.contract-sizes/CompositeLiquidityRouter
#	pkg/vault/test/.contract-sizes/Router
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
4e25851 
# Conflicts:
#	pkg/vault/test/.contract-sizes/CompositeLiquidityRouter
jubeira
jubeira reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @EndymionJkb, looks good!
I have a few comments and minor suggestions; I think I'd just modify the routers and keep the vault as is.

pkg/vault/contracts/Router.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/Router.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/RouterCommon.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/Vault.sol Outdated Show resolved Hide resolved
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
5a1199c
@EndymionJkb
refactor: revert change to main Vault - don't check in sendTo - try…
87b44a3 
… to manage outside. Unfortunately need to do it in VaultAdmin then
@EndymionJkb
refactor: Router changes to avoid calling Vault functions that would …
106a93f 
…do zero transfers
@EndymionJkb
chore: update bytecode
d85dcfd
@EndymionJkb
refactor: avoid making Vault calls in the test framework that do zero…
8e148ac 
… transfers
@EndymionJkb
test: update tests to avoid zero transfers
f70d306
@EndymionJkb
chore: update gas
b562bac
@EndymionJkb
lint
78a2808
@EndymionJkb
refactor: don't need to check for zero transfer of weth
51ec01c
@EndymionJkb
chore: update bytecode
976b94f
@EndymionJkb
Merge branch 'main' into zero-xfer-v2
cd2c1b5 
# Conflicts:
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - Standard - BatchRouter] swap exact in with one token and fees - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate - BatchRouter] swap exact in with one token and fees - cold slots
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - With rate] remove liquidity single token exact out - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] swap exact in with one token and fees - cold slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate - BatchRouter] swap exact in with one token and fees - warm slots
#	pkg/vault/test/.contract-sizes/BatchRouter
#	pkg/vault/test/.contract-sizes/CompositeLiquidityRouter
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard - BatchRouter] add liquidity unbalanced using swapExactIn - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard] add liquidity single token exact out - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard] remove liquidity single token exact out - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard] swap single token exact in with fees - cold slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - Standard] swap single token exact in with fees - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMock - With rate - BatchRouter] remove liquidity using swapExactOut - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - ERC4626 - BatchRouter] swapExactIn - no buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - ERC4626 - BatchRouter] swapExactOut - with buffer liquidity - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - Standard - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - With rate - BatchRouter] remove liquidity using swapExactIn - warm slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - With rate - BatchRouter] swap exact in with one token and fees - cold slots
#	pkg/vault/test/gas/.hardhat-snapshots/[PoolMockWithHooks - With rate - BatchRouter] swap exact in with one token and fees - warm slots
@EndymionJkb
refactor: don't call settle with 0
40f6b35
@EndymionJkb
test: update settle in test
99f8951
@EndymionJkb
chore: update bytecode
0c3947b
@EndymionJkb
chore: update gas
4ed09f9
jubeira
jubeira approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@jubeira jubeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good; just a few minor comments. Thanks for pushing this @EndymionJkb !

Do you still think it'd make sense to protect sendTo against zero value inside the vault? I was initially against it, and in general it's better to avoid the external call from the router by checking outside sendTo. I'm leaning towards keeping it like this (i.e. no protection inside sendTo, as you should avoid those calls anyways), but I'm open to other opinions.

pkg/pool-hooks/contracts/MinimalRouter.sol
IERC20 token = tokens[i];

// There can be only one WETH token in the pool.
if (params.wethIsEth && address(token) == address(_weth)) {
// Send WETH here and unwrap to native ETH.
_vault.sendTo(_weth, address(this), amountOut);
_weth.withdraw(amountOut);
ethAmountOut = amountOut;
// Send ETH to receiver.
payable(params.receiver).sendValue(amountOut);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good idea.

pkg/vault/contracts/BatchRouter.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/test/PoolHooksMock.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/test/PoolHooksMock.sol Outdated Show resolved Hide resolved
@EndymionJkb
Copy link
Collaborator Author

Do you still think it'd make sense to protect sendTo against zero value inside the vault? I was initially against it, and in general it's better to avoid the external call from the router by checking outside sendTo. I'm leaning towards keeping it like this (i.e. no protection inside sendTo, as you should avoid those calls anyways), but I'm open to other opinions.

I'd rather not change it all back again at this point. It's probably a decent trade-off to have a little extra router code to avoid pointless external calls. It's an edge case anyway. Ideally we wouldn't have had to change the Vault at all - but there is that one place in VaultAdmin that calls sendTo directly.

@EndymionJkb EndymionJkb merged commit 59ed949 into main Oct 1, 2024
12 checks passed
@EndymionJkb EndymionJkb deleted the zero-xfer-v2 branch October 1, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubeira jubeira jubeira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle tokens that prevent zero transfer
2 participants
@EndymionJkb @jubeira