Float Capital contest - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

• The exponent in the comment is 5, but its value is set to 52. Also since it is a constant, it should be in ALL_CAPS

// With an exponent of 5, the largest total liquidity possible in a market (to avoid integer overflow on exponentiation) is ~10^31 or 10 Trillion (10^13)
// NOTE: this also means if the total market value is less than 2^52 there will be a division by zero error
uint256 public constant safeExponentBitShifting = 52;

• The comment in _changeBalanceIncentiveExponent() says that the exponent has to be less than 5 // The exponent has to be less than 5 in these versions of the contracts., but it allows the value 5 to be set.

  _balanceIncentiveCurve_exponent > 0 && _balanceIncentiveCurve_exponent < 6,

  Perhaps saying that it has to be strictly less than 6, or at most 5, would provide better clarity.

Recommended Mitigation Steps

// With an exponent of 52...
uint256 public constant SAFE_EXPONENT_BIT_SHIFTING = 52

require(
	// The exponent has to be at most 5 in these versions of the contracts.
  _balanceIncentiveCurve_exponent > 0 && _balanceIncentiveCurve_exponent < 6,
  "balanceIncentiveCurve_exponent out of bounds"
);

Handle

hickuphh3

Vulnerability details

Impact

For critical user actions like minting, redeeming, shifting and staking synthetic assets, consider adding re-entrancy protections.

Handle

hickuphh3

Vulnerability details

Impact

Updating a kValue of a market requires interpolation against the initial timestamp, which can be a hassle and might lead to a wrong value set from what is expected.

Proof of Concept

Consider the following scenario:

• Initially set kValue = 2e18, kPeriod = 2592000 (30 days)
• After 15 days, would like to refresh the market incentive (start again with kValue = 2e18), lasting another 30 days.

In the current implementation, the admin would call _changeMarketLaunchIncentiveParameters() with the following inputs:

• period = 3888000 (45 days)

• kValue needs to be worked backwards from the formula

  kInitialMultiplier - (((kInitialMultiplier - 1e18) * (block.timestamp - initialTimestamp)) / kPeriod). To achieve the desired effect, we would get kValue = 25e17 (formula returns 2e18 after 15 days with kPeriod = 45 days).

This isn't immediately intuitive and could lead to mistakes.

Recommended Mitigation Steps

Instead of calculating from initialTimestamp (when addNewStakingFund() was called), calculate from when the market incentives were last updated. This would require a new mapping to store last updated timestamps of market incentives.

For example, using the scenario above, refreshing the market incentive would mean using inputs period = 2592000 (30 days) with kValue = 2e18.

// marketIndex => timestamp of updated market launch incentive params 
mapping(uint32 => uint256) public marketLaunchIncentive_update_timestamps;

function _changeMarketLaunchIncentiveParameters(
  uint32 marketIndex,
  uint256 period,
  uint256 initialMultiplier
) internal virtual {
	require(initialMultiplier >= 1e18, "marketLaunchIncentiveMultiplier must be >= 1e18");

  marketLaunchIncentive_period[marketIndex] = period;
  marketLaunchIncentive_multipliers[marketIndex] = initialMultiplier;
	marketLaunchIncentive_update_timestamps[marketIndex] = block.timestamp;
};

function _getKValue(uint32 marketIndex) internal view virtual returns (uint256) {
  // Parameters controlling the float issuance multiplier.
  (uint256 kPeriod, uint256 kInitialMultiplier) = _getMarketLaunchIncentiveParameters(marketIndex);

  // Sanity check - under normal circumstances, the multipliers should
  // *never* be set to a value < 1e18, as there are guards against this.
  assert(kInitialMultiplier >= 1e18);

	// currently: uint256 initialTimestamp = accumulativeFloatPerSyntheticTokenSnapshots[marketIndex][0].timestamp;
	// changed to take from last updated timestamp instead of initial timestamp
  uint256 initialTimestamp = marketLaunchIncentive_update_timestamps[marketIndex];

  if (block.timestamp - initialTimestamp <= kPeriod) {
    return kInitialMultiplier - (((kInitialMultiplier - 1e18) * (block.timestamp - initialTimestamp)) / kPeriod);
  } else {
    return 1e18;
  }
}

Handle

hickuphh3

Vulnerability details

Impact

TokenFactory.sol defines DEFAULT_ADMIN_ROLE = keccak256("DEFAULT_ADMIN_ROLE");, but OpenZeppelin's AccessControl.sol defines DEFAULT_ADMIN_ROLE = 0x00, so that by default, all other roles defined will have their admin role to be DEFAULT_ADMIN_ROLE.

This makes the following lines erroneous:

// Give minter roles
SyntheticToken(syntheticToken).grantRole(DEFAULT_ADMIN_ROLE, longShort);
SyntheticToken(syntheticToken).grantRole(MINTER_ROLE, longShort);
SyntheticToken(syntheticToken).grantRole(PAUSER_ROLE, longShort);

// Revoke roles
SyntheticToken(syntheticToken).revokeRole(DEFAULT_ADMIN_ROLE, address(this));
SyntheticToken(syntheticToken).revokeRole(MINTER_ROLE, address(this));
SyntheticToken(syntheticToken).revokeRole(PAUSER_ROLE, address(this));

Due to how grantRole() and revokeRole() works, the lines above will not revert. However, note that TokenFactory will have DEFAULT_ADMIN_ROLE (0x00) instead of LongShort. This by itself doesn't seem to have any adverse effects, since TokenFactory doesn't do anything else apart from creating new synthetic tokens.

Nonetheless, I believe that DEFAULT_ADMIN_ROLE was unintentionally defined as keccak256("DEFAULT_ADMIN_ROLE"), and should be amended.

The revoking role order will also have to be swapped so that DEFAULT_ADMIN_ROLE is revoked last.

Recommended Mitigation Steps

bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;

function createSyntheticToken(
  string calldata syntheticName,
  string calldata syntheticSymbol,
  address staker,
  uint32 marketIndex,
  bool isLong
) external override onlyLongShort returns (ISyntheticToken syntheticToken) {
	...
  // Revoke roles
  _syntheticToken.revokeRole(MINTER_ROLE, address(this));
  _syntheticToken.revokeRole(PAUSER_ROLE, address(this));
	_syntheticToken.revokeRole(DEFAULT_ADMIN_ROLE, address(this));
}

Handle

hickuphh3

Vulnerability details

Impact

In the unlikely event amountReservedInCaseOfInsufficientAaveLiquidity == amount, the else case will be executed, which means lendingPool.deposit() is called with a value of zero. It would therefore be better to change the condition so that the if case is executed instead.

Recommended Mitigation Steps

function depositPaymentToken(uint256 amount) external override longShortOnly {
  // If amountReservedInCaseOfInsufficientAaveLiquidity isn't zero, then efficiently net the difference between the amount
  // It basically always be zero besides extreme and unlikely situations with aave.
  if (amountReservedInCaseOfInsufficientAaveLiquidity != 0) {
		// instead of strictly greater than
    if (amountReservedInCaseOfInsufficientAaveLiquidity >= amount) {
      amountReservedInCaseOfInsufficientAaveLiquidity -= amount;
      // Return early, nothing to deposit into the lending pool
      return;
    }
	...
}

Handle

hickuphh3

Vulnerability details

Impact

The following variables can be made immutable to save gas.

TokenFactory.sol

• longShort

SyntheticToken.sol

• longShort
• staker
• marketIndex
• isLong

YieldManagerAave.sol

• longShort
• treasury
• paymentToken
• aToken
• lendingPool
• aaveIncentivesController
• referralCode

Handle

hickuphh3

Vulnerability details

Impact

The README states that "the synthetic token is written to never return false." as it inherits from OpenZeppelin's ERC20PresetMinterPauser.

It is also claimed that "We only check the return boolean (success) for erc20 methods on the payment token not for the synthetic token", but this is not true. LongShort.sol does in fact check that the transfer() and transferFrom() methods returns true (L855 - 857, 900-906, 961-963, 1015-1020).

Recommended Mitigation Steps

Since synthetic tokens have been engineered to always return true, consider dropping the require checks to save gas.

Handle

hickuphh3

Vulnerability details

Impact

The number of solc runs used for contract compilation is 200. This number can be bumped significantly to produce more gas efficient code (max value of 2**32-1).

More information can be found in the solidity docs.

Recommended Mitigation Steps

In hardhat.config.js, increase solc runs significantly. Contract sizes and thus deployment costs will increase, but functions will cost less gas to execute.

Handle

hickuphh3

Vulnerability details

Impact

By storing marketUpdateIndex[marketIndex]; locally in _updateSystemStateInternal(), multiple sLOADs can be avoided.

Recommended Mitigation Steps

Gas savings of about 500-600 is achieved.

function _updateSystemStateInternal(uint32 marketIndex) internal virtual requireMarketExists(marketIndex) {
	...
	// cache marketUpdateIndex[marketIndex]
  uint256 currentMarketIndex = marketUpdateIndex[marketIndex];
	
	bool assetPriceHasChanged = oldAssetPrice != newAssetPrice;

	if (assetPriceHasChanged || msg.sender == staker) {
		uint256 syntheticTokenPrice_inPaymentTokens_long = syntheticToken_priceSnapshot[marketIndex][true][
			currentMarketIndex
    ];
    uint256 syntheticTokenPrice_inPaymentTokens_short = syntheticToken_priceSnapshot[marketIndex][false][
      currentMarketIndex
    ];

		if (
      userNextPrice_currentUpdateIndex[marketIndex][staker] == currentMarketIndex + 1 &&
      assetPriceHasChanged
    ) {
      IStaker(staker).pushUpdatedMarketPricesToUpdateFloatIssuanceCalculations(
        marketIndex,
        syntheticTokenPrice_inPaymentTokens_long,
        syntheticTokenPrice_inPaymentTokens_short,
        marketSideValueInPaymentToken[marketIndex][true],
        marketSideValueInPaymentToken[marketIndex][false],
        // This variable could allow users to do any next price actions in the future (not just synthetic side shifts)
        currentMarketIndex + 1
      );
    } else {
      IStaker(staker).pushUpdatedMarketPricesToUpdateFloatIssuanceCalculations(
        marketIndex,
        syntheticTokenPrice_inPaymentTokens_long,
        syntheticTokenPrice_inPaymentTokens_short,
        marketSideValueInPaymentToken[marketIndex][true],
        marketSideValueInPaymentToken[marketIndex][false],
        0
      );
    }
		...
		// increment currentMarketIndex
		currentMarketIndex ++;
	  marketUpdateIndex[marketIndex] = currentMarketIndex;
		syntheticToken_priceSnapshot[marketIndex][true][
			currentMarketIndex
	  ] = syntheticTokenPrice_inPaymentTokens_long;
	
	  syntheticToken_priceSnapshot[marketIndex][false][
	    currentMarketIndex
	  ] = syntheticTokenPrice_inPaymentTokens_short;
		...
		emit SystemStateUpdated(
	    marketIndex,
	    currentMarketIndex,
			...
		);
	}
}

Handle

hickuphh3

Vulnerability details

Impact

In getYieldSplit(),

• since longValue and shortValue is already compared and stored as a boolean in isLongSideUnderbalanced, calculating their difference can be unchecked.
• the same can be done for the calculation of treasuryYieldPercent_e18, since marketPercent_e18 is capped to 1e18

Recommended Mitigation Steps

isLongSideUnderbalanced = longValue < shortValue;
uint256 imbalance;

unchecked {
	imbalance = isLongSideUnderbalanced ? shortValue - longValue : longValue - shortValue;
}

unchecked { treasuryYieldPercent_e18 = 1e18 - marketPercent_e18; }

Handle

hickuphh3

Vulnerability details

Impact

The user's userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_long and userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_short are retrieved a number of times in _calculateAccumulatedFloat(). Caching these values would help save gas.

Note that block scoping is needed to avoid the stack too deep problem.

Recommended Mitigation Steps

function _calculateAccumulatedFloat() {
	// block scope for shiftAmount variable to avoid stack too deep
	{
		// Update the users balances
	  uint256 shiftAmount = userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_long[marketIndex][user];
	  if (shiftAmount > 0) {
	    amountStakedShort += ILongShort(longShort).getAmountSyntheticTokenToMintOnTargetSide(
	      marketIndex,
	      shiftAmount,
	      true,
	      stakerTokenShiftIndex_to_longShortMarketPriceSnapshotIndex_mapping[usersShiftIndex]
	    );
	
	    amountStakedLong -= shiftAmount;
	    userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_long[marketIndex][user] = 0;
	  }
	
	  shiftAmount = userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_short[marketIndex][user]
	  if (shiftAmount > 0) {
	    amountStakedLong += ILongShort(longShort).getAmountSyntheticTokenToMintOnTargetSide(
	      marketIndex,
	      shiftAmount,
	      false,
	      stakerTokenShiftIndex_to_longShortMarketPriceSnapshotIndex_mapping[usersShiftIndex]
	    );
	
	    amountStakedShort -= shiftAmount;
	    userNextPrice_amountStakedSyntheticToken_toShiftAwayFrom_short[marketIndex][user] = 0;
	  }
	}
	// end of block scoping

	// Save the users updated staked amounts
	...
}

Handle

hickuphh3

Vulnerability details

Impact

The lines

accumulativeFloatPerSyntheticTokenSnapshots[marketIndex][0].accumulativeFloatPerSyntheticToken_long = 0;
accumulativeFloatPerSyntheticTokenSnapshots[marketIndex][0].accumulativeFloatPerSyntheticToken_short = 0;

are redundant since their default value is zero.

Recommended Mitigation Steps

Remove L355 and 356.

Handle

hickuphh3

Vulnerability details

Impact

In createSyntheticToken(), syntheticToken is casted numerous to call the grantRole() and revokeRole() functions. This can be avoided by declaring a SyntheticToken type in memory, utilising that variable, then casting it to the return argument.

It would also be more appropriate to return ISyntheticToken as the output to further reduce the need for casting in LongShort.sol.

Recommended Mitigation Steps

function createSyntheticToken(
  string calldata syntheticName,
  string calldata syntheticSymbol,
  address staker,
  uint32 marketIndex,
  bool isLong
) external override onlyLongShort returns (ISyntheticToken syntheticToken) {
	SyntheticToken _syntheticToken = 
    new SyntheticToken(syntheticName, syntheticSymbol, longShort, staker, marketIndex, isLong);

  // Give minter roles
  _syntheticToken.grantRole(DEFAULT_ADMIN_ROLE, longShort);
  _syntheticToken.grantRole(MINTER_ROLE, longShort);
  _syntheticToken.grantRole(PAUSER_ROLE, longShort);

  // Revoke roles
  _syntheticToken.revokeRole(DEFAULT_ADMIN_ROLE, address(this));
  _syntheticToken.revokeRole(MINTER_ROLE, address(this));
  _syntheticToken.revokeRole(PAUSER_ROLE, address(this));

	syntheticToken = ISyntheticToken(address(_syntheticToken));
}

Handle

hickuphh3

Vulnerability details

Impact

By declaring appropriate variable types to storage variables, unnecessary / fewer type casting can be avoided.

Recommended Mitigation Steps

LongShort.sol

1. mapping(uint32 => int256) public assetPrice;
2. mapping(uint32 => IYieldManager) public yieldManagers;
3. mapping(uint32 => IERC20) public paymentTokens;
4. mapping(uint32 => IOracleManager) public oracleManagers;
5. mapping(uint32 => mapping(bool => ISyntheticToken)) public syntheticTokens;

SyntheticToken.sol

1. IStaker public staker;

Staker.sol

1. IFloatToken public floatToken;
2. ILongShort public longShort;