Skip to content

Commit

Permalink
Audit Fixes: QS-3 (#64)
Browse files Browse the repository at this point in the history 
* QS-3: Introduced depositTs to prevent rewards inflation

* Used variable to access depositTs

* Updated test cases

* perf(test): Removed redundant test

* Renamed error

---------

Co-authored-by: Yash Pitroda <yash.pitroda786@gmail.com>
  • Loading branch information
@arcantheon @YashP16
arcantheon and YashP16 authored Jun 21, 2024
1 parent a98fa09 commit e1359d8
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 19 deletions.
18 changes: 15 additions & 3 deletions contracts/Farm.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions contracts/features/OperableDeposit.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions contracts/interfaces/DataTypes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
22 changes: 22 additions & 0 deletions test/Farm.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
43 changes: 43 additions & 0 deletions test/e20-farms/E20Farm.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -261,6 +303,7 @@ abstract contract DecreaseDepositTest is E20FarmTest {

abstract contract E20FarmInheritTest is
E20FarmDepositTest,
E20FarmWithdrawTest,
IncreaseDepositTest,
RecoverERC20E20FarmTest,
DecreaseDepositTest
Expand Down
28 changes: 17 additions & 11 deletions test/e721-farms/camelotV3/CamelotV3Farm.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);
}
Expand All @@ -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)]);
}
Expand All @@ -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));
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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));
Expand All @@ -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();
Expand All @@ -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,
Expand Down
15 changes: 10 additions & 5 deletions test/e721-farms/uniswapv3/UniV3Farm.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);
}
Expand All @@ -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)]);
}
Expand All @@ -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));
Expand All @@ -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();
Expand All @@ -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,
Expand Down

0 comments on commit e1359d8

Please sign in to comment.