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."...

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...

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...

initializeMarket uses wrong market index for synthetic

Handle cmichel Vulnerability details The LongShort.initializeMarket function accepts a marketIndex parameter to identify which market should be initialized. However, this index is not used everywhere, when calling IStakerstaker.addNewStakingFund the latestMarket variable is used. In the...

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...

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...

What is the right place for amountReservedInCaseOfInsufficientAaveLiquidity in transferPaymentTokensToUser?

Handle pauliax Vulnerability details Impact transferPaymentTokensToUser is called from LongShort's executeOutstandingNextPriceRedeems which in turn is called from executeOutstandingNextPriceSettlements which is quite a common function so I don't think that always subtracting the amount from...

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...

Short shift is not cleared

Handle cmichel Vulnerability details The LongShort.batchConfirmOutstandingPendingActions function uses the batchedamountSyntheticTokentoShiftAwayFrommarketSidemarketIndexfalse field to determine how much short tokens to shift to long tokens. However, this field is not cleared, instead, the long...

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 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...

Incorrect shift variable is reset in LongShort

Handle shw Vulnerability details Impact The logic at lines 1187 to 1208 of LongShort is to handle the user's shift from SHORT to LONG. However, at line 1207, the number of tokens to be shifted from LONG is reset to 0 instead of SHORT, causing incorrect accounting of LONG and SHORT tokens. Proof o...

Missing events/timelocks for owner/admin only functions that change critical parameters

Handle 0xRajeev Vulnerability details Impact Owner/admin only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate the...

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 ;...

OracleManagerFlippening_V0 price data could be stale

Handle cmichel Vulnerability details There is no check in OracleManagerFlippeningV0.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 balance computed in getUsersConfirmedButNotSettledSynthBalance()

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

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...

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 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...

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...

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...

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...

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...

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...

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...

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,...

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...

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...

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...

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...

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...

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...

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...

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...

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...

_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...

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...

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...

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...

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...

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...

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...

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...

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...

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...

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...