Incorrect balance computed in getUsersConfirmedButNotSettledSynthBalance()

Handle hack3r-0m Vulnerability details Consider the following state: longsynthbalace = 300; shortsynthbalace = 200; marketUpdateIndex1 = x; userNextPricecurrentUpdateIndex = 0; userNextPricesyntheticTokentoShiftAwayFrommarketSide1true = 0; batchedamountSyntheticTokentoShiftAwayFrommarketSide1true...

Users could shift tokens on Staker with more than he has staked

Handle shw Vulnerability details Impact The shiftTokens function of Staker checks whether the user has staked at least the number of tokens he wants to shift from one side to the other line 885. A user could call the shiftTokens function multiple times before the next price update to shift the...

initialMarket always initialize the latest market

Handle jonah1005 Vulnerability details Impact In longshort contract's initializeMarket, while it should initialize according to the parameter marketIndex, it initialize latestMarket. This would break two market, the market of marketIndex' and the latest market. User's fund would get stuck at the...

OracleManagerFlippening_V0, OracleManagerEthVsBtc only work if chainlink decimals are the same

Handle cmichel Vulnerability details The OracleManagerFlippeningV0, OracleManagerEthVsBtc contracts divide the ETH price by the BTC price and want to return a value where 100% = 1e20. This does not work if the chainlink oracle decimals for ETH are different from the one for BTC. Impact The "price...

Single-step process for critical admin transfer is risky

Handle 0xRajeev Vulnerability details Impact LongShort and Staker contracts have the notion of an “admin” address that is used within onlyAdmin or adminOnly modifiers for granting authorization to critical functions. Such contracts use a single-step ownership transfer of such admin addresses usin...

OracleManagerEthKillerChainlink price data could be stale

Handle cmichel Vulnerability details There is no check in OracleManagerEthKillerChainlink.getLatestPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation: under current notifications: "if answeredInRound roundId could indicate stal...

OracleManagerEthVsBtc price data could be stale

Handle cmichel Vulnerability details There is no check in OracleManagerEthVsBtc.getLatestPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation: under current notifications: "if answeredInRound roundId could indicate stale data."...

getUsersConfirmedButNotSettledSynthBalance uses current price instead of deposit price

Handle cmichel Vulnerability details The LongShort.getUsersConfirmedButNotSettledSynthBalance function estimates the outstanding synthetic tokens using the latest synthetic token price currentMarketUpdateIndex, instead of the price at the next price update after the user deposited...

OracleManagerFlippening_V0 wrong decimals

Handle cmichel Vulnerability details The OracleManagerFlippeningV0.updatePrice function states that it wants to return the eth dominance as a percentage where 100% = 1e20. It's unclear why there is a division by 1e10 to compute the bitcoin market cap: uint256btcPrice btcSupply 1e10 Impact The pri...

Incorrect usage of latestMarket in _seedMarketInitially and initializeMarket

Handle pauliax Vulnerability details Impact function seedMarketInitially in LongShort contract uses latestMarket when minting the initial tokens: ISyntheticTokensyntheticTokenslatestMarkettrue.mint PERMANENTINITIALLIQUIDITYHOLDER, initialMarketSeedForEachMarketSide ;...

Unchecked return values from transfer()

Handle JMukesh Vulnerability details Impact It is usually good to add a require-statement that checks the return value or to use something like safeTransfer; unless one is sure the given token reverts in case of a failure. Proof of Concept Tools Used manual review Recommended Mitigation Steps use...

Incorrect use of latestMarket instead of marketIndex in several functions of LongShort

Handle shw Vulnerability details Impact Some part of the logic in the initializeMarket and seedMarketInitially functions of LongShort incorrectly operates on the latestMarket instead of marketIndex, the provided parameter. Since the latestMarket is not necessary to be the market to be initialized...

ERC20 return values not checked

Handle cmichel Vulnerability details The ERC20.transfer and ERC20.transferFrom functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead. The Staker.withdraw function does not chec...

OracleManagerChainlink price data could be stale

Handle cmichel Vulnerability details There is no check in OracleManagerChainlink.getLatestPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation: under current notifications: "if answeredInRound roundId could indicate stale data."...

Incorrect parameters passed while adding new staking fund

Handle hack3r-0m Vulnerability details initializeMarket can be called with different marketIndex each time while calling IStakerstaker.addNewStakingFund with the same parameters resulting in overriding of mapping in the staker contract and hence removing past staking funds. latestMarket should be...

Wrong aave usage of claimRewards

Handle jonah1005 Vulnerability details Impact Aave yield manager claims rewards with the payment token. According to aave's document, aToken should be provided. The aave rewards would be unclaimable. Proof of Concept YieldManager's logic: Reference: Tools Used None Recommended Mitigation Steps...

Potential underflow on userAmountStaked[token][msg.sender] in _withdraw

Handle 0xImpostor Vulnerability details Impact Underflowing userAmountStakedtokenmsg.sender once will let me exploit the entire token balance in the Staker contract. This can only be exploited if marketUnstakeFeee18 is ≥ 50%. Proof of Concept 1. Admin sets marketUnstakeFeee18 for this marketIndex...

initializeMarket always initialize the latest market

Handle jonah1005 Vulnerability details Impact In longshore contract's initializeMarket, while it should initialize according to the parameter marketIndex, it initialize latestMarket. This would break two market, the market of marketIndex' and market of latestMarket. User's fund would get stuck at...

Potential for market to be created but never initialized

Handle loop Vulnerability details Impact Multiple markets can be created before being initialized since createNewSyntheticMarket and initializeMarket are separate functions. The SyntheticTokens used in initialization will however always be those of the latest market created. Proof of Concept Let'...

Potential div by 0

Handle 0xImpostor Vulnerability details Impact The staking contract will fail because you are unable to calculate float per second as it is trying to divide by 0. Potential div by 0 is found here i.e. totalLocked safeExponentBitShifting You are trying to right shift totalLocked by 52 bits so if...

Synths minted to the wrong market when initializing

Handle 0xImpostor Vulnerability details Impact Synthetix tokens are not minted to the correct market index since the creation of the synth market and the initialization are 2 separate steps. Proof of Concept 1. Create 2 synth market without initializing them 2. Call initializeMarket twice 3. Synt...

getUsersConfirmedButNotSettledSynthBalance is potentially calculated wrongly

Handle 0xImpostor Vulnerability details Impact Incorrect tabulation of getUsersConfirmedButNotSettledSynthBalance will lead to the wrong balances returning. Fortunately, there are no important functions that are dependent on balanceOf so the impact of this erroneous calculation is limited. Tools...

User is still able to frontrun

Handle evertkors Vulnerability details Impact An attempt to solve front-running attacks by using the nextPrice model is not effective. Users are still able to execute a front-running attack as the time of the next price execution is arbitrary. The oracle is called at an arbitrary point in time...

Prevent markets getting stuck when prices don't move

Handle gpersoon Vulnerability details Impact Suppose there is a synthetic token where the price stays constant, for example: synthetic DAI with a payment token of DAI the price will not move binary option token for example tracking the USA elections; after the election results there will be no mo...

Staker.sol: Wrong values returned in edge cases of _calculateFloatPerSecond()

Handle hickuphh3 Vulnerability details Impact In calculateFloatPerSecond, the edge cases where full rewards go to either the long or short token returns return 1e18 k longPrice, 0; and return 0, 1e18 k shortPrice; respectively. This is however 1e18 times too large. We can verify this by checking...

2 variables not indexed by marketIndex

Handle gpersoon Vulnerability details Impact In the token contract: batchedstakerNextTokenShiftIndex is indexed by marketIndex, so it can have separate or the same values for each different marketIndex. stakerTokenShiftIndextolongShortMarketPriceSnapshotIndexmapping and...

latestMarket used where marketIndex should have been used

Handle gpersoon Vulnerability details Impact The functions initializeMarket and seedMarketInitially use the variable latestMarket. If these functions would be called seperately from createNewSyntheticMarket, then latestMarket would have the same value for each call of initializeMarket and...

Admin and treasury change should be confirmed.

Handle tensors Vulnerability details Impact Inputting the wrong address here could lock out a lot of the funds and smart contract methods. Proof of Concept Recommended Mitigation Steps Require the changed address to confirm the switch with a pendingAdmin, pendingTreasury variable. --- The text wa...

copy paste error in _batchConfirmOutstandingPendingActions

Handle gpersoon Vulnerability details Impact The function batchConfirmOutstandingPendingActions of LongShort.sol processeses the variable batchedamountSyntheticTokentoShiftAwayFrommarketSide, and sets it to 0 after processing. However probably due to a copy/paste error, in the second instance,...

ambiguous transferFrom

Handle jonah1005 Vulnerability details Impact In function redeemToken, it uses transferFrom instead of transfer. It might stop users from redeeming tokens. Proof of Concept Tools Used None Recommended Mitigation Steps Use safeTransfer instead. --- The text was updated successfully, but these erro...

Yield sources cannot be swapped back

Handle shw Vulnerability details Impact The setYieldSource function of SwappableYieldSource calls the safeApprove function to approve the yield sources with the maximum allowance of transferring underlying tokens. However, according to OpenZeppelin's implementation, the safeApprove function...

Usage of safeApprove

Handle pauliax Vulnerability details Impact function approveMax uses safeApprove. This function only works if the current approval is 0. Consider clearing previous approval safeApprove0 before setting the max value again. The same issue can happen with SwappableYieldSource if, for example, source...

SwappableYieldSource: Missing same deposit token check in transferFunds()

Handle hickuphh3 Vulnerability details Impact transferFunds will transfer funds from a specified yield source yieldSource to the current yield source set in the contract currentYieldSource. However, it fails to check that the deposit tokens are the same. If the specified yield source's assets are...

Use of safeApprove will always cause approveMax to revert

Handle 0xRajeev Vulnerability details Impact Unlike SwappableYieldSource which uses safeIncreaseAllowance to increase the allowance to uint256.max, mStableYieldSource uses OpenZeppelin’s safeApprove which has been documented as 1 Deprecated because of approve-like race condition and 2 To be used...

_setYieldSource does not reset approval

Handle cmichel Vulnerability details The SwappableYieldSource.setYieldSource function approves the new yield source but does not reset the old yield source's approval to zero. It could be the case that the old yield source is compromised or has a bug and could therefore transfer and steal any...

Inconsistent balance when supplying transfer-on-fee or deflationary tokens

Handle shw Vulnerability details Impact The supplyTokenTo function of SwappableYieldSource assumes that amount of depositToken is transferred to itself after calling the safeTransferFrom function and thus it supplies amount of token to the yield source. However, this may not be true if the...

Incorrect address check in transferERC20 can allow rugging

Handle 0xRajeev Vulnerability details Impact SwappableYieldSource.sol has a transferERC20 function callable only by the owner or asset manager to transfer out ERC20 tokens other than the yield source's tokens held by this contract. This is similar to the functions in ATokenYieldSource and...

onlyOwnerOrAssetManager can swap Yield Source in SwappableYieldSource at any time, immediately rugging all funds from old yield source

Handle GalloDaSballo Vulnerability details Impact The function swapYieldSource Can be called by the owner deployer / initializer or Asset Manager The function will take all funds from the old Yield Source, and transfer them to the new Yield source. Any contract that implement the function functio...

The approveMax function of MStableYieldSource always reverts

Handle shw Vulnerability details Impact The approveMax function of MStableYieldSource calls the safeApprove function to set the allowance to the maximum. However, at the time of call, the allowance should be non-zero since it was set to the maximum in the constructor function. The non-zero...

Deposits don't work with fee-on transfer tokens

Handle cmichel Vulnerability details There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer or transferFrom. Others are rebasing tokens that increase in value over time like...

redeemToken sends tokens with safeTransferFrom

Handle pauliax Vulnerability details Impact function redeemToken sends tokens to the msg.sender by using safeTransferFrom: depositToken.safeTransferFromaddressthis, msg.sender, redeemableBalance; For safeTransferFrom to work it needs to have an enough approval. In this case, obviously this contra...

Single-step process for critical ownership transfer/renounce is risky

Handle 0xRajeev Vulnerability details Impact The SwappableYieldSource allows owners and asset managers to set/swap/transfer yield sources/funds. As such, the contract ownership plays a critical role in the protocol. Given that AssetManager is derived from Ownable, the ownership management of this...

redeemToken can fail for certain tokens

Handle cmichel Vulnerability details The SwappableYieldSource.redeemToken function transfers tokens from the contract back to the sender, however, it uses the ERC20.transferFromaddressthis, msg.sender, redeemableBalance function for this. Some deposit token implementations might fail as...

Transfer-on-fee/deflationary tokens are not correctly accounted for

Handle shw Vulnerability details Impact When a user stakes or a protocol deposits a transfer-on-fee/deflationary token, the solution does not correctly handle the received amount, which could be less than what is accounted for. Proof of Concept Referenced code: PoolOpen.solL36-L38...

supplyTokenTo doesn't account for safeTransferFrom fees

Handle gpersoon Vulnerability details Impact The function supplyTokenTo of MStableYieldSource retrieves the tokens from the msg.sender and deposits them. However some tokens, like USDT might subtract a fee when transferring tokens. This means less tokens would be transferred than expected. If thi...

setYieldSource leads to temporary wrong results

Handle gpersoon Vulnerability details Impact The use of setYieldSource leaves the contract in a temporary inconsistent state because it changes the underlying yield source, but doesn't yet transfer the underlying balances, while the shares stay the same. The function balanceOfToken will show the...

reputation risk via upgradable contracts

Handle gpersoon Vulnerability details Impact The contract SwappableYieldSource is upgradable. This means the owner could upgrade and change the contract so any new functionality. Amongst others the owner could retrieve all the tokens of the Yieldsource and transfer them out. The project could sti...

Old yield source still has infinite approval

Handle tensors Vulnerability details Impact After swapping a yield source, the old yield source still has infinite approval. Infinite approval has been used in large attacks if the yield source isn't perfectly safe see furucombo. Proof of Concept Recommended Mitigation Steps Decrease approval aft...

Difficult for the project to be decentralized if the Watsons share one address.

Handle tensors Vulnerability details Impact The Watsons share a single address. As it stands right now the Watsons could be a single person effectively providing insurance with other peoples risk. There should be mechanisms in place to make sure Watson's have an accurate amount of skin in the gam...

TokenToLock default value

Handle cmichel Vulnerability details The PoolBase.TokenToLockXRate function returns the "Current exchange rate from token to lockToken". It does not specify the precision and according to the documentation, it sounds like one just has to multiply this value by any token amount to get the...