diff --git a/contracts/Farm.sol b/contracts/Farm.sol index b5b7d53b..bab5864c 100644 --- a/contracts/Farm.sol +++ b/contracts/Farm.sol @@ -83,6 +83,7 @@ abstract contract Farm is FarmStorage, Ownable, ReentrancyGuard, Initializable, error InvalidAddress(); error ZeroAmount(); error InvalidCooldownPeriod(); + error WithdrawTooSoon(); // Disallow initialization of a implementation contract. constructor() Ownable(msg.sender) { @@ -431,10 +432,11 @@ abstract contract Farm is FarmStorage, Ownable, ReentrancyGuard, Initializable, // Prepare data to be stored. Deposit memory userDeposit = Deposit({ depositor: _account, - cooldownPeriod: 0, + liquidity: _liquidity, expiryDate: 0, - totalRewardsClaimed: new uint256[](rewardTokens.length), - liquidity: _liquidity + cooldownPeriod: 0, + depositTs: block.timestamp, + totalRewardsClaimed: new uint256[](rewardTokens.length) }); // @dev Pre increment because we want deposit IDs to start with 1. @@ -489,6 +491,7 @@ abstract contract Farm is FarmStorage, Ownable, ReentrancyGuard, Initializable, // Note: If farm is paused, skip the cooldown check. if (isFarmActive()) { Deposit storage userDeposit = deposits[_depositId]; + _validateNotRecentDeposit(userDeposit.depositTs); if (userDeposit.cooldownPeriod != 0) { revert PleaseInitiateCooldown(); } @@ -767,6 +770,15 @@ abstract contract Farm is FarmStorage, Ownable, ReentrancyGuard, Initializable, } } + /// @notice A function to validate deposit ts to prevent flash loan vulnerabilities + /// @param _depositTs depositTs of user's deposit. (It represents deposit ts or increaseDeposit ts) + /// @dev Reverts when deposit made in the same transaction. + function _validateNotRecentDeposit(uint256 _depositTs) internal view { + if (_depositTs == block.timestamp) { + revert WithdrawTooSoon(); + } + } + /// @notice Validate if farm is open. Revert otherwise. /// @dev This function can be overridden to add any new/additional logic. function _validateFarmOpen() internal view { diff --git a/contracts/features/OperableDeposit.sol b/contracts/features/OperableDeposit.sol index a755a0d3..63492a85 100644 --- a/contracts/features/OperableDeposit.sol +++ b/contracts/features/OperableDeposit.sol @@ -103,6 +103,9 @@ abstract contract OperableDeposit is ExpirableFarm { _updateSubscriptionForIncrease(_depositId, _amount); userDeposit.liquidity += _amount; + // Update depositTs to prevent flash loan vulnerabilities + userDeposit.depositTs = block.timestamp; + emit DepositIncreased(_depositId, _amount); } @@ -112,6 +115,7 @@ abstract contract OperableDeposit is ExpirableFarm { //Validations. _validateFarmOpen(); // Withdraw instead of decrease deposit when farm is closed. _validateDeposit(msg.sender, _depositId); + _validateNotRecentDeposit(userDeposit.depositTs); if (_amount == 0) { revert CannotWithdrawZeroAmount(); diff --git a/contracts/interfaces/DataTypes.sol b/contracts/interfaces/DataTypes.sol index 2ea086ac..c4d22444 100644 --- a/contracts/interfaces/DataTypes.sol +++ b/contracts/interfaces/DataTypes.sol @@ -35,12 +35,14 @@ struct Subscription { // liquidity - amount of liquidity in the deposit. // expiryDate - expiry time (if deposit is locked). // cooldownPeriod - cooldown period in days (if deposit is locked). +// depositTs - Timestamp of deposit, increaseDeposit saved to prevent deposit and withdrawals in same tx. // totalRewardsClaimed - total rewards claimed for the deposit. struct Deposit { address depositor; uint256 liquidity; uint256 expiryDate; uint256 cooldownPeriod; + uint256 depositTs; uint256[] totalRewardsClaimed; } diff --git a/test/Farm.t.sol b/test/Farm.t.sol index 19c7473f..7e1d88e9 100644 --- a/test/Farm.t.sol +++ b/test/Farm.t.sol @@ -289,6 +289,7 @@ abstract contract WithdrawTest is FarmTest { assertEq(depositInfo.liquidity, 0); assertEq(depositInfo.expiryDate, 0); assertEq(depositInfo.cooldownPeriod, 0); + assertEq(depositInfo.depositTs, 0); } function _assertHelperTwo( @@ -308,6 +309,7 @@ abstract contract WithdrawTest is FarmTest { assertEq(depositInfo.liquidity, 0); assertEq(depositInfo.expiryDate, 0); assertEq(depositInfo.cooldownPeriod, 0); + assertEq(depositInfo.depositTs, 0); vm.expectRevert(abi.encodeWithSelector(Farm.SubscriptionDoesNotExist.selector)); Farm(farm).getSubscriptionInfo(i, 0); @@ -341,10 +343,22 @@ abstract contract WithdrawTest is FarmTest { useKnownActor(user) { uint256 depositId = 1; + skip(1); vm.expectRevert(abi.encodeWithSelector(Farm.PleaseInitiateCooldown.selector)); Farm(lockupFarm).withdraw(depositId); } + function test_Withdraw_RevertWhen_DepositInSameTs() + public + setup + depositSetup(lockupFarm, true) + useKnownActor(user) + { + uint256 depositId = 1; + vm.expectRevert(abi.encodeWithSelector(Farm.WithdrawTooSoon.selector)); + Farm(lockupFarm).withdraw(depositId); + } + function test_Withdraw_RevertWhen_DepositIsInCooldown() public setup @@ -429,6 +443,8 @@ abstract contract WithdrawTest is FarmTest { if (lockup) { Farm(farm).initiateCooldown(depositId); skip(cooldownTime); //100 seconds after the end of CoolDown Period + } else { + skip(1); } Farm(farm).getRewardBalance(rwdTokens[0]); Farm(farm).getDepositInfo(depositId); @@ -476,6 +492,8 @@ abstract contract WithdrawTest is FarmTest { if (lockup) { Farm(farm).initiateCooldown(withdrawnDepositId); skip(cooldownTime); //100 seconds after the end of CoolDown Period + } else { + skip(1); } Farm(farm).getRewardBalance(rwdTokens[0]); Farm(farm).getDepositInfo(withdrawnDepositId); @@ -526,6 +544,8 @@ abstract contract WithdrawTest is FarmTest { if (lockup) { Farm(farm).initiateCooldown(withdrawnDepositId); skip(cooldownTime); //100 seconds after the end of CoolDown Period + } else { + skip(1); } Farm(farm).getRewardBalance(rwdTokens[0]); Farm(farm).getDepositInfo(withdrawnDepositId); @@ -576,6 +596,8 @@ abstract contract WithdrawTest is FarmTest { if (lockup) { Farm(farm).initiateCooldown(withdrawnDepositId); skip(cooldownTime); //100 seconds after the end of CoolDown Period + } else { + skip(1); } Farm(farm).getRewardBalance(rwdTokens[0]); Farm(farm).getDepositInfo(withdrawnDepositId); diff --git a/test/e20-farms/E20Farm.t.sol b/test/e20-farms/E20Farm.t.sol index 5a7577f4..9046d91f 100644 --- a/test/e20-farms/E20Farm.t.sol +++ b/test/e20-farms/E20Farm.t.sol @@ -49,6 +49,23 @@ abstract contract E20FarmDepositTest is E20FarmTest { } } +abstract contract E20FarmWithdrawTest is E20FarmTest { + function test_revertWhen_withdraw_withdrawInSameTransactionAsIncrease() + public + depositSetup(lockupFarm, true) + useKnownActor(user) + { + address poolAddress = getPoolAddress(); + uint256 amt = 500 * 10 ** ERC20(poolAddress).decimals(); + + deal(poolAddress, user, amt); + ERC20(poolAddress).approve(address(lockupFarm), amt); + E20Farm(lockupFarm).increaseDeposit(DEPOSIT_ID, amt); + vm.expectRevert(abi.encodeWithSelector(Farm.WithdrawTooSoon.selector)); + E20Farm(lockupFarm).withdraw(DEPOSIT_ID); + } +} + abstract contract IncreaseDepositTest is E20FarmTest { event DepositIncreased(uint256 indexed depositId, uint256 liquidity); @@ -177,8 +194,33 @@ abstract contract RecoverERC20E20FarmTest is E20FarmTest { abstract contract DecreaseDepositTest is E20FarmTest { event DepositDecreased(uint256 indexed depositId, uint256 liquidity); + function test_revertWhen_decreaseDeposit_decreaseInSameTransactionAsIncrease() + public + depositSetup(lockupFarm, true) + useKnownActor(user) + { + address poolAddress = getPoolAddress(); + uint256 amt = 500 * 10 ** ERC20(poolAddress).decimals(); + + deal(poolAddress, user, amt); + ERC20(poolAddress).approve(address(lockupFarm), amt); + E20Farm(lockupFarm).increaseDeposit(DEPOSIT_ID, amt); + vm.expectRevert(abi.encodeWithSelector(Farm.WithdrawTooSoon.selector)); + E20Farm(lockupFarm).decreaseDeposit(DEPOSIT_ID, amt); + } + + function test_decreaseDeposit_decreaseInSameTransactionAsDeposit() + public + depositSetup(lockupFarm, true) + useKnownActor(user) + { + vm.expectRevert(abi.encodeWithSelector(Farm.WithdrawTooSoon.selector)); + E20Farm(lockupFarm).decreaseDeposit(DEPOSIT_ID, AMOUNT); + } + function test_zeroAmount() public depositSetup(lockupFarm, true) useKnownActor(user) { uint256 amount; + skip(1); vm.expectRevert(abi.encodeWithSelector(Farm.CannotWithdrawZeroAmount.selector)); E20Farm(lockupFarm).decreaseDeposit(DEPOSIT_ID, amount); } @@ -261,6 +303,7 @@ abstract contract DecreaseDepositTest is E20FarmTest { abstract contract E20FarmInheritTest is E20FarmDepositTest, + E20FarmWithdrawTest, IncreaseDepositTest, RecoverERC20E20FarmTest, DecreaseDepositTest diff --git a/test/e721-farms/camelotV3/CamelotV3Farm.t.sol b/test/e721-farms/camelotV3/CamelotV3Farm.t.sol index 062bc95d..79e8c30c 100644 --- a/test/e721-farms/camelotV3/CamelotV3Farm.t.sol +++ b/test/e721-farms/camelotV3/CamelotV3Farm.t.sol @@ -697,6 +697,7 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { depositSetup(lockupFarm, true) useKnownActor(user) { + skip(1); vm.expectRevert(abi.encodeWithSelector(Farm.CannotWithdrawZeroAmount.selector)); CamelotV3Farm(lockupFarm).decreaseDeposit(depositId, 0, [uint256(0), uint256(0)]); } @@ -706,6 +707,7 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { depositSetup(lockupFarm, true) useKnownActor(user) { + skip(1); vm.expectRevert(abi.encodeWithSelector(OperableDeposit.DecreaseDepositNotPermitted.selector)); CamelotV3Farm(lockupFarm).decreaseDeposit(depositId, dummyLiquidityToWithdraw, [uint256(0), uint256(0)]); } @@ -715,6 +717,7 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { address farm; farm = isLockupFarm ? lockupFarm : nonLockupFarm; depositSetupFn(farm, false); + skip(1); uint128 oldLiquidity = SafeCast.toUint128(CamelotV3Farm(farm).getDepositInfo(depositId).liquidity); uint128 liquidityToWithdraw = SafeCast.toUint128(bound(_liquidityToWithdraw, 1, oldLiquidity)); @@ -726,8 +729,9 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { uint256[2] memory minAmounts = [uint256(0), uint256(0)]; uint256 oldCommonTotalLiquidity = CamelotV3Farm(farm).getRewardFundInfo(CamelotV3Farm(farm).COMMON_FUND_ID()).totalLiquidity; - uint256 oldUserToken0Balance = IERC20(DAI).balanceOf(currentActor); - uint256 oldUserToken1Balance = IERC20(USDCe).balanceOf(currentActor); + CamelotV3Farm(farm).claimRewards(depositId); + IERC20(DAI).transfer(makeAddr("Random"), IERC20(DAI).balanceOf(currentActor)); + IERC20(USDCe).transfer(makeAddr("Random"), IERC20(USDCe).balanceOf(currentActor)); vm.expectEmit(farm); emit DepositDecreased(depositId, liquidityToWithdraw); @@ -752,8 +756,8 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { } assertTrue(found, "DecreaseLiquidity event not found"); assertEq(loggedLiquidity, liquidityToWithdraw); - assertEq(IERC20(DAI).balanceOf(currentActor), oldUserToken0Balance + loggedAmount0); - assertEq(IERC20(USDCe).balanceOf(currentActor), oldUserToken1Balance + loggedAmount1); + assertEq(IERC20(DAI).balanceOf(currentActor), loggedAmount0); + assertEq(IERC20(USDCe).balanceOf(currentActor), loggedAmount1); assertEq(CamelotV3Farm(farm).getDepositInfo(depositId).liquidity, oldLiquidity - liquidityToWithdraw); assertEq( CamelotV3Farm(farm).getRewardFundInfo(CamelotV3Farm(farm).COMMON_FUND_ID()).totalLiquidity, @@ -774,6 +778,7 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { address farm; farm = isLockupFarm ? lockupFarm : nonLockupFarm; depositSetupFn(farm, false); + skip(1); uint128 oldLiquidity = SafeCast.toUint128(CamelotV3Farm(farm).getDepositInfo(depositId).liquidity); uint128 liquidityToWithdraw = SafeCast.toUint128(bound(_liquidityToWithdraw, 1, oldLiquidity)); @@ -785,8 +790,9 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { uint256[2] memory minAmounts = [uint256(0), uint256(0)]; uint256 oldCommonTotalLiquidity = CamelotV3Farm(farm).getRewardFundInfo(CamelotV3Farm(farm).COMMON_FUND_ID()).totalLiquidity; - uint256 oldUserToken0Balance = IERC20(DAI).balanceOf(currentActor); - uint256 oldUserToken1Balance = IERC20(USDCe).balanceOf(currentActor); + CamelotV3Farm(farm).claimRewards(depositId); + IERC20(DAI).transfer(makeAddr("Random"), IERC20(DAI).balanceOf(currentActor)); + IERC20(USDCe).transfer(makeAddr("Random"), IERC20(USDCe).balanceOf(currentActor)); vm.stopPrank(); address camelotFactoryOwner = ICamelotV3FactoryTesting(CAMELOT_V3_FACTORY).owner(); @@ -809,19 +815,19 @@ abstract contract DecreaseDepositTest is CamelotV3FarmTest { uint128 loggedLiquidity; uint256 loggedAmount0; uint256 loggedAmount1; - // bool found = false; + bool found = false; for (uint256 i = 0; i < entries.length; i++) { if (entries[i].topics[0] == keccak256("DecreaseLiquidity(uint256,uint128,uint256,uint256)")) { (loggedLiquidity, loggedAmount0, loggedAmount1) = abi.decode(entries[i].data, (uint128, uint256, uint256)); - // found = true; + found = true; } } - // assertTrue(found, "DecreaseLiquidity event not found"); + assertTrue(found, "DecreaseLiquidity event not found"); assertEq(loggedLiquidity, liquidityToWithdraw); - assertEq(IERC20(DAI).balanceOf(currentActor), oldUserToken0Balance + loggedAmount0); - assertEq(IERC20(USDCe).balanceOf(currentActor), oldUserToken1Balance + loggedAmount1); + assertEq(IERC20(DAI).balanceOf(currentActor), loggedAmount0); + assertEq(IERC20(USDCe).balanceOf(currentActor), loggedAmount1); assertEq(CamelotV3Farm(farm).getDepositInfo(depositId).liquidity, oldLiquidity - liquidityToWithdraw); assertEq( CamelotV3Farm(farm).getRewardFundInfo(CamelotV3Farm(farm).COMMON_FUND_ID()).totalLiquidity, diff --git a/test/e721-farms/uniswapv3/UniV3Farm.t.sol b/test/e721-farms/uniswapv3/UniV3Farm.t.sol index c7951ef1..4fe5448b 100644 --- a/test/e721-farms/uniswapv3/UniV3Farm.t.sol +++ b/test/e721-farms/uniswapv3/UniV3Farm.t.sol @@ -640,6 +640,7 @@ abstract contract DecreaseDepositTest is UniV3FarmTest { depositSetup(lockupFarm, true) useKnownActor(user) { + skip(1); vm.expectRevert(abi.encodeWithSelector(Farm.CannotWithdrawZeroAmount.selector)); UniV3Farm(lockupFarm).decreaseDeposit(depositId, 0, [uint256(0), uint256(0)]); } @@ -649,6 +650,7 @@ abstract contract DecreaseDepositTest is UniV3FarmTest { depositSetup(lockupFarm, true) useKnownActor(user) { + skip(1); vm.expectRevert(abi.encodeWithSelector(OperableDeposit.DecreaseDepositNotPermitted.selector)); UniV3Farm(lockupFarm).decreaseDeposit(depositId, dummyLiquidityToWithdraw, [uint256(0), uint256(0)]); } @@ -658,6 +660,7 @@ abstract contract DecreaseDepositTest is UniV3FarmTest { address farm; farm = isLockupFarm ? lockupFarm : nonLockupFarm; depositSetupFn(farm, false); + skip(1); uint128 oldLiquidity = SafeCast.toUint128(UniV3Farm(farm).getDepositInfo(depositId).liquidity); uint128 liquidityToWithdraw = SafeCast.toUint128(bound(_liquidityToWithdraw, 1, oldLiquidity)); @@ -669,14 +672,16 @@ abstract contract DecreaseDepositTest is UniV3FarmTest { uint256[2] memory minAmounts = [uint256(0), uint256(0)]; uint256 oldCommonTotalLiquidity = UniV3Farm(farm).getRewardFundInfo(UniV3Farm(farm).COMMON_FUND_ID()).totalLiquidity; - uint256 oldUserToken0Balance = IERC20(DAI).balanceOf(currentActor); - uint256 oldUserToken1Balance = IERC20(USDCe).balanceOf(currentActor); + UniV3Farm(farm).claimRewards(depositId); + IERC20(DAI).transfer(makeAddr("Random"), IERC20(DAI).balanceOf(currentActor)); + IERC20(USDCe).transfer(makeAddr("Random"), IERC20(USDCe).balanceOf(currentActor)); + assertEq(IERC20(DAI).balanceOf(currentActor), 0); + assertEq(IERC20(USDCe).balanceOf(currentActor), 0); vm.expectEmit(farm); emit DepositDecreased(depositId, liquidityToWithdraw); vm.expectEmit(true, false, false, false, NFPM); emit DecreaseLiquidity(tokenId, 0, 0, 0); - vm.recordLogs(); UniV3Farm(farm).decreaseDeposit(depositId, liquidityToWithdraw, minAmounts); VmSafe.Log[] memory entries = vm.getRecordedLogs(); @@ -695,8 +700,8 @@ abstract contract DecreaseDepositTest is UniV3FarmTest { } assertTrue(found, "DecreaseLiquidity event not found"); assertEq(loggedLiquidity, liquidityToWithdraw); - assertEq(IERC20(DAI).balanceOf(currentActor), oldUserToken0Balance + loggedAmount0); - assertEq(IERC20(USDCe).balanceOf(currentActor), oldUserToken1Balance + loggedAmount1); + assertEq(IERC20(DAI).balanceOf(currentActor), loggedAmount0); + assertEq(IERC20(USDCe).balanceOf(currentActor), loggedAmount1); assertEq(UniV3Farm(farm).getDepositInfo(depositId).liquidity, oldLiquidity - liquidityToWithdraw); assertEq( UniV3Farm(farm).getRewardFundInfo(UniV3Farm(farm).COMMON_FUND_ID()).totalLiquidity,