From aa5dddb9ab996c7226a5293975164a7480167f64 Mon Sep 17 00:00:00 2001 From: danoctavian Date: Sat, 14 Sep 2024 04:31:30 +0300 Subject: [PATCH 1/3] add fuzz test to verify deposit vs withdrawal --- test/single/integration/ynBNB.bsc.t.sol | 42 ++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/test/single/integration/ynBNB.bsc.t.sol b/test/single/integration/ynBNB.bsc.t.sol index 27f2565..cba069d 100644 --- a/test/single/integration/ynBNB.bsc.t.sol +++ b/test/single/integration/ynBNB.bsc.t.sol @@ -31,7 +31,7 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe function setUp() public onlyBsc { slis = IERC20(slisBNB); - get_slisBNB(address(this), 10_000 ether); + get_slisBNB(address(this), 100_000 ether); factory = VaultFactory(address(VAULT_FACTORY)); slis.approve(address(factory), 1 ether); slis.transfer(address(factory), 1 ether); @@ -90,4 +90,44 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe slis.approve(address(kslis), 1 ether); ksup.deposit(address(kslis), 1 ether, 1 ether); } + + function test_ynBNB_deposit_withdraw_sameAmount_fuzz( + uint256 amount, + uint256 rewards + ) public onlyBsc { + address user = USER; + vm.assume(amount > 0 && amount <= 10000 ether); + vm.assume(rewards >= 0 && rewards <= 10000 ether); + + slis.approve(user, amount); + slis.transfer(user, amount); + + slis.transfer(address(ynBNB), rewards); + + vm.startPrank(user); + slis.approve(address(ynBNB), amount); + uint256 shares = ynBNB.deposit(amount, user); + + assertEq(ynBNB.totalSupply(), shares + 1 ether, "Total supply should equal shares plus initial 1 ether"); + assertEq(ynBNB.totalAssets(), amount + rewards + 1 ether, "Total assets should equal shares plus initial 1 ether"); + vm.stopPrank(); + + // Withdraw shares and assert received amount + vm.startPrank(user); + uint256 preWithdrawBalance = slis.balanceOf(user); + uint256 assetsReceived = ynBNB.redeem(shares, user, user); + uint256 postWithdrawBalance = slis.balanceOf(user); + + assertEq(postWithdrawBalance - preWithdrawBalance, assetsReceived, "User balance delta should equal assetsReceived"); + + assertGe(amount, assetsReceived, "Amount deposited should be greater than or equal to assets received"); + + uint256 assetLoss = amount - assetsReceived; + uint256 maxLoss = (amount > rewards ? amount: rewards) / 1e18; + assertLe(assetLoss, maxLoss + 2, "Asset loss should be less than or equal to maxLoss"); + + assertEq(ynBNB.totalSupply(), 1 ether, "Total supply should return to initial 1 ether"); + assertEq(ynBNB.totalAssets(), rewards + 1 ether + assetLoss, "Total assets should return to initial 1 ether"); + vm.stopPrank(); + } } From f08de81e34feed91aad7acb6b2b8400cb21b033e Mon Sep 17 00:00:00 2001 From: xhad Date: Sat, 14 Sep 2024 11:03:46 +0800 Subject: [PATCH 2/3] Prevent upgrade unit test from running during integration --- test/single/integration/ynBNB.bsc.t.sol | 6 +++--- test/single/unit/invariants.sol | 2 +- test/single/unit/upgrade.t.sol | 7 ++++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/test/single/integration/ynBNB.bsc.t.sol b/test/single/integration/ynBNB.bsc.t.sol index cba069d..5c6652d 100644 --- a/test/single/integration/ynBNB.bsc.t.sol +++ b/test/single/integration/ynBNB.bsc.t.sol @@ -19,10 +19,10 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe VaultFactory public factory; SingleVault public ynBNB; IERC20 public slis; - IERC4626 kslis = IERC4626(KARAK_KslisBNB); - IKarakVaultSupervisor ksup = IKarakVaultSupervisor(KARAK_VAULT_SUPERVISOR); + IERC4626 public kslis = IERC4626(KARAK_KslisBNB); + IKarakVaultSupervisor public ksup = IKarakVaultSupervisor(KARAK_VAULT_SUPERVISOR); - address USER = 0x0c099101d43e9094E4ae9bC2FC38f8b9875c23c5; + address public USER = 0x0c099101d43e9094E4ae9bC2FC38f8b9875c23c5; modifier onlyBsc() { if (block.chainid != 56) return; diff --git a/test/single/unit/invariants.sol b/test/single/unit/invariants.sol index 030b989..3d06fd9 100644 --- a/test/single/unit/invariants.sol +++ b/test/single/unit/invariants.sol @@ -16,7 +16,7 @@ contract SingleInvariantTests is Test, LocalActors, TestConstants { SingleVault public vault; MockERC20 public asset; - address USER = address(33); + address public USER = address(33); function setUp() public { vm.startPrank(ADMIN); diff --git a/test/single/unit/upgrade.t.sol b/test/single/unit/upgrade.t.sol index f123f8d..413b9ed 100644 --- a/test/single/unit/upgrade.t.sol +++ b/test/single/unit/upgrade.t.sol @@ -32,7 +32,12 @@ contract SingleVaultUpgradeTests is Test, LocalActors, TestConstants { factory = IVaultFactory(setup.factory()); } - function testUpgrade() public { + modifier onlyLocal() { + if (block.chainid != 31337) return; + _; + } + + function testUpgrade() public onlyLocal { vm.startPrank(ADMIN); SingleVault newVault = new MockSingleVault(); From 328c907c977133fac08f0e61a13dc03c8e05409c Mon Sep 17 00:00:00 2001 From: xhad Date: Sat, 14 Sep 2024 11:18:50 +0800 Subject: [PATCH 3/3] forge fmt --- README.md | 5 +++++ test/single/integration/ynBNB.bsc.t.sol | 17 +++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 6ded122..d6d09aa 100644 --- a/README.md +++ b/README.md @@ -100,3 +100,8 @@ export EXECUTOR_2="your_executor_2" make single-vault ``` +## Security Notes + +There's a loss incurred by the user that's roughly less than `Max(amount, rewards) / 1e18` when he withdraws the exact same shares he received when compared to what he deposited. + +I believe this is because of that decimal offset in OZ 46426Upgradeable to prevent donation attacks. The loss maxes out at 10000 wei for 10000 ether amounts so it's small. \ No newline at end of file diff --git a/test/single/integration/ynBNB.bsc.t.sol b/test/single/integration/ynBNB.bsc.t.sol index 5c6652d..1167887 100644 --- a/test/single/integration/ynBNB.bsc.t.sol +++ b/test/single/integration/ynBNB.bsc.t.sol @@ -91,12 +91,9 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe ksup.deposit(address(kslis), 1 ether, 1 ether); } - function test_ynBNB_deposit_withdraw_sameAmount_fuzz( - uint256 amount, - uint256 rewards - ) public onlyBsc { + function test_ynBNB_deposit_withdraw_sameAmount_fuzz(uint256 amount, uint256 rewards) public onlyBsc { address user = USER; - vm.assume(amount > 0 && amount <= 10000 ether); + vm.assume(amount > 0 && amount <= 10000 ether); vm.assume(rewards >= 0 && rewards <= 10000 ether); slis.approve(user, amount); @@ -109,7 +106,9 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe uint256 shares = ynBNB.deposit(amount, user); assertEq(ynBNB.totalSupply(), shares + 1 ether, "Total supply should equal shares plus initial 1 ether"); - assertEq(ynBNB.totalAssets(), amount + rewards + 1 ether, "Total assets should equal shares plus initial 1 ether"); + assertEq( + ynBNB.totalAssets(), amount + rewards + 1 ether, "Total assets should equal shares plus initial 1 ether" + ); vm.stopPrank(); // Withdraw shares and assert received amount @@ -118,12 +117,14 @@ contract KslisBNB_Test is Test, BscActors, BscContracts, ynBNBConstants, AssetHe uint256 assetsReceived = ynBNB.redeem(shares, user, user); uint256 postWithdrawBalance = slis.balanceOf(user); - assertEq(postWithdrawBalance - preWithdrawBalance, assetsReceived, "User balance delta should equal assetsReceived"); + assertEq( + postWithdrawBalance - preWithdrawBalance, assetsReceived, "User balance delta should equal assetsReceived" + ); assertGe(amount, assetsReceived, "Amount deposited should be greater than or equal to assets received"); uint256 assetLoss = amount - assetsReceived; - uint256 maxLoss = (amount > rewards ? amount: rewards) / 1e18; + uint256 maxLoss = (amount > rewards ? amount : rewards) / 1e18; assertLe(assetLoss, maxLoss + 2, "Asset loss should be less than or equal to maxLoss"); assertEq(ynBNB.totalSupply(), 1 ether, "Total supply should return to initial 1 ether");