transferFrom() and shareKey() doesn't check for _from != _recipient

Handle GiveMeTestEther Vulnerability details Impact / POC There is no check from != recipient in transferFrom https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinTransfer.solL109 the key manager/approver can expire the k...

_transferBurn should reduce totalSupply

Handle WatchPug Vulnerability details function transferBurn address sender, address recipient, uint256 amount, uint256 burnt internal uint256 senderBalance = balancessender; requiresenderBalance = amount + burnt, "OVL:balance= amount + burnt, "OVL:balanceamount+burnt"; unchecked balancessender =...

Transmuter yield can be gamed by only staking when yield is distributed

Handle cmichel Vulnerability details The Transmuter.distribute function distributes the yield to the buffer which is then distributed to all stakers over a TRANSMUTATIONPERIOD, see runPhasedDistribution and updateAccount. If the elapsed time from the last phase distribution is greater than the...

No way to remove GasThrottle after deployment

Handle TomFrench Vulnerability details Impact Potential DOS on swaps Proof of Concept BasePool and BasePoolV2 make use of a validateGas modifier on swaps which checks that the user's gas price is below the value returned by FASTGASORACLE. Should FASTGASORACLE be compromised to always return zero...

Early user can break addLiquidity

Handle WatchPug Vulnerability details uint256 totalLiquidityUnits = totalSupply; if totalLiquidityUnits == 0 liquidity = nativeDeposit; // TODO: Contact ThorChain on proper approach In the current implementation, the first liquidity takes the nativeDeposit amount and uses it directly. However,...

Zap contract's redeem() function might transfer more tokens than the user should be able to receive

Handle Ruhum Vulnerability details Impact The redeem function transfers the whole balance of the specified token to the user. That can be more than the amount the user should receive. Because at the time of the call to redeem the Zap contract's balance of the passed token might not be 0. Proof of...

LinearVesting does not calculate vested amount linearly

Handle xYrYuYx Vulnerability details Impact These calculations are incorrect for linear vesting. Proof of Concept i.e. if start amount is 10000, and duration is 100 seconds. After 50 seconds, user can claim 5000 which is 50% After another 10 seconds, user need to claim 1000 which is 10%, but...

FSDVesting: Restrict updateVestedTokens() calls to only FSD token contract

Handle hickuphh3 Vulnerability details Impact The updateVestedTokens increases the amount of tokens to be vested for a beneficiary. There is no access restriction to the function. The intended total vesting duration is 30 months with a 12-month cliff where 5% is immediately unlocked, and the...

Unbounded loop in TwapOracle.update can result in oracle being locked

Handle TomFrench Vulnerability details Impact Loss of ability of TwapOracle to update should too many pools be added. Proof of Concept TwapOracle allows an unlimited number of pairs to be added and has no way of removing pairs after the fact. At the same time TwapOracle.update iterates through al...

Slingshot: Incorrect initial balance fetched for native token in executeTrades()

Handle hickuphh3 Vulnerability details Impact The executioner contract only supports ERC20ERC20 token trades. Native token swaps are supported by either wrapping / unwrapping the ERC20 wrapped native token before / after the trades respectively. When exchanging from the native token, the wrapping...

Leftover balance in the Executioner contract can be drained

Handle gzeon Vulnerability details Impact Leftover balance in the Executioner contract can be drained by swapping the target assetnative/erc20 into another asset. Slingshot.executeTrades allow user to execute trade using modules as long as the module is registered in the ModuleRegistry. The...

Lack Of Return Value Check On the Oracle

Handle defsec Vulnerability details Impact During the code review, It has been seen that oracle return value has not been checked on the function. If oracle is returned price as a 0, balance of shares will be zero. Proof of Concept 1. Navigate to "" 2. The return value pricePerShare function has...

UserManager: updateLockedData() doesn't check that the amount is actually locked.

Handle itsmeSTYJ Vulnerability details Impact The function updateLockedData does not actually check if the amount required to be locked is actually locked. Proof of Concept Same solution as my other high issue. I've added comments where relevant. function updateLockedData address borrower, uint25...

allocatedTokensPerEpoch cannot be changed under special scenario

Handle csanuragjain Vulnerability details Impact allocatedTokensPerEpoch will fail to change and higher rewards would be given. Proof of Concept 1. Navigate to 2. Check the setAllocatedTokensPerEpoch function function setAllocatedTokensPerEpochuint128 amount public onlyOwner requireamount 0,...

Incorrect updateGlobalExchangeRate implementation

Handle xYrYuYx Vulnerability details Impact UpdateGlobalExchangeRate has incorrect implementation when totalGlobalShares is zero. If any user didn't start stake, totalGlobalShares is 0, and every stake it will increase. but there is possibility that totalGlobalShares can be 0 amount later by...

Prevent execution with invalid signatures

Handle gpersoon Vulnerability details Impact Suppose one of the supplied addrsi to the constructor of Identity.sol happens to be 0 by accident. In that case: privileges0 = 1 Now suppose you call execute with an invalid signature, then recoverAddrImpl will return a value of 0 and thus signer=0. If...

rebalance function will fail due to invalid condition

Handle csanuragjain Vulnerability details Impact User will be unable to rebalance the fund Proof of Concept 1. Navigate to contract at 2. Check the rebalance function function rebalanceaddress tokenAddress, uint256 calldata percentages external override checkMarketSupportedtokenAddress onlyAdmin...

DDOS by cancelling orders or removing approvals

Handle cmichel Vulnerability details The market assumes that the maker of an order approved the swivel contract as there are many calls that move funds from the maker to the contract, see Swivel.initiateVaultFillingZcTokenInitiate/initiateZcTokenFillingVaultInitiate: uToken.transferFromo.maker,...

Swivel: Taker is charged fees twice in exitVaultFillingVaultInitiate

Handle itsmeSTYJ Vulnerability details Impact Taker is charged fees twice in exitVaultFillingVaultInitiate . Maker is transferring less than premiumFilled to taker and then taker is expected to pay fees i.e. taker's net balance is premiumFilled - 2fee Recommended Mitigation Steps function...

Incorrect multiplication in _computeSingleOutGivenPoolIn of IndexPool

Handle broccoli Vulnerability details Impact The computeSingleOutGivenPoolIn function of IndexPool uses the raw multiplication i.e., to calculate the zaz variable. However, since both BASE - normalizedWeight and swapFee are in WAD, the mul function should be used instead to calculate the correct...

Funds in the pool could be stolen by exploiting flashSwap in HybridPool

Handle broccoli Vulnerability details Impact An attacker can call the bento.harvest function during the callback function of a flash swap of the HybridPool to reduce the number of input tokens that he has to pay to the pool, as long as there is any unrealized profit in the strategy contract of th...

PostAuctionLauncher.sol#finalize() Adding liquidity to an existing pool may allows the attacker to steal most of the tokens

Handle WatchPug Vulnerability details PostAuctionLauncher.finalize can be called by anyone, and it sends tokens directly to the pair pool to mint liquidity, even when the pair pool exists. An attacker may control the LP price by creating the pool and then call finalize to mint LP token with unfai...

earn results in decreasing share price

Handle jonah1005 Vulnerability details Impact For a dai vault that pairs with NativeStrategyCurve3Crv, every time earn is called, shareholders would lose money. about 2% There're two issues involved. The Vault contract and the controller contract doesn't handle the price difference between the wa...

Missing validation on latestRoundData

Handle adelamo Vulnerability details On ExchangeRate.sol, we are using latestRoundData, but there are no validations that the data is not stale. The current code is: / uint80 /, rate, / uint256 /, / uint256 /, / uint80 / = AggregatorV2V3InterfacerateOracle.latestRoundData; requirerate 0,...

ChainLink price data could be stale

Handle cmichel Vulnerability details Vulnerability Details There is no check in ExchangeRate.buildExchangeRate 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 indicat...

Can't call external functions internally

Handle tensors Vulnerability details Impact Within GovernerAlpha.sol, certain functions are locked because of the improper modifier. addressthis cannot call external functions in lines L470-497. Proof of Concept Recommended Mitigation Steps Change external to internal. --- The text was updated...

Owner has a rugpull function

Handle tensors Vulnerability details Impact The owner of the contract has a rugpull function. This can be unsafe if the private key for the owner account falls into the wrong hands, allowing instant withdrawal of all the funds. In general, having a single point of failure like this is not...

findNewOwner edgecase

Handle gpersoon Vulnerability details Impact In the function findNewOwner of RCOrderbook, as loop is done which included the check loopCounter maxDeletions Afterwards a check is done for "loopCounter != maxDeletions" to determine if the processing is finished. If loopCounter == maxDeletions then...

Exchange rates from Compound are assumed with 18 decimals

Handle shw Vulnerability details Impact The CTokenMultiOracle contract assumes the exchange rates borrowing rate of Compound always have 18 decimals, while, however, which is not true. According to the Compound documentation, the exchange rate returned from the exchangeRateCurrent function is...

Unchecked return value 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 che...

ERC20Rewards returns wrong rewards if no tokens initially exist

Handle cmichel Vulnerability details The ERC20Rewards.updateRewardsPerToken function exits without updating rewardsPerToken.lastUpdated if totalSupply is zero, i.e., if there are no tokens initially. This leads to an error if there is an active rewards period but not tokens have been minted yet...

No slippage protection in Strategy

Handle cmichel Vulnerability details The startPool and endPool functions of Strategy.sol mint/burn tokens in the pool without any minimum return amount checks: // startPool pool.mintaddressthis, true, 0 // endPool ,, uint256 fyTokenDivested = pool.burnaddressthis, 0, 0; If one of the pool tokens ...

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

Free stake by replacing input token address

Handle jonah1005 Vulnerability details Impact In PoolOpen contract, pools' authentication is done through baseData. However, it handles token transfer based on the given parameter token. By appending a real token address at the end of the transaction and a fake address in the function parameter,...

Return values of BEP20.transfer and BEP20.transferFrom are unchecked

Handle shw Vulnerability details Impact The return values of BEP20.transfer and BEP20.transferFrom are not checked to be true in multiple contracts. The return value could be false if the transferred token is not BEP20-compliant, indicating that the transfer fails, while the calling contract will...

Flash loan manipulation on getPoolShareWeight of Utils

Handle shw Vulnerability details Impact The getPoolShareWeight function returns a user's pool share weight by calculating how many SPARTAN the user's LP tokens account for. However, this approach is vulnerable to flash loan manipulation since an attacker can swap a large number of TOKEN to SPARTA...

Dao.calcReward(address) has potential division before multiplication arithmetic errors

Handle heiho1 Vulnerability details Impact Dao.calcRewardaddress on lines 203 and 204 are potentially problematic in that division may potentially truncate values resulting in loss of precision. Proof of Concept Tools Used Slither Recommended Mitigation Steps Potentially this issue is lessened wi...

[Pool] - Flash loan + Synth.realise allows anyone to extract value from LPs

Handle adelamo Vulnerability details Impact On Synth.sol, we have the method realise that checks if the LP value is higher than the Synth value. If confirmed, it will burn the premium LP. Using a flash loan, we can add liquidity to the pool, mint some LP tokens. Then, call realise using the pool...

Router.addDividend(address,uint256) has potentially unsafe arithmetic

Handle heiho1 Vulnerability details Impact Router.addDividendaddress,uint256 has potentially unsafe division before multiplication. This could lead to truncation. Proof of Concept Tools Used Slither Recommended Mitigation Steps Potentially this issue is mitigated by recent Solidity changes to...

SynthVault withdraw forfeits rewards

Handle cmichel Vulnerability details Vulnerability Details The SynthVault.withdraw function does not claim the user's rewards. It decreases the user's weight and therefore they are forfeiting their accumulated rewards. The synthReward variable in processWithdraw is also never used - it was probab...

Signature replay attacks are possible if a user's transactionId is not unique

Handle shw Vulnerability details Impact If a user's two transactions can have the same transactionId while their router addresses differ, then the fulfill and cancel signatures for one of the transaction is replayable on the other. Proof of Concept The only transaction-related data in the...

Expired transfers will lock user funds on the sending chain

Handle 0xRajeev Vulnerability details Impact The cancelling relayer is being paid in receivingAssetId on the sendingChain instead of in sendingAssetID. If the user relies on a relayer to cancel transactions and that receivingAssetId asset does not exist on the sending chain assuming only...

Use of deprecated Chainlink function latestAnswer

Handle shw Vulnerability details Impact According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0, causing an incorrect price fed to the Buoy3Pool. Proof of Concept Referenced code: Buoy3Pool.solL207...

grief a user by not allowing him to retrieve funds

Handle gpersoon Vulnerability details Impact The function removeUserActiveBlocks contains a "for" loop, which depends on the size of the array activeTransactionBlocks. If the array is too large then the for loop will take so much gas that the transaction will revert. The function fulfill, which...

Unchecked token transfers

Handle Lucius Vulnerability details Impact The functions transerFrom/transer do not revert on failure and instead simply return false. Without checks on the return values, the transfers could potentially fail silently allowing unexpected issues with certain token pools. E.G. If a user calls depos...

Locked funds are debited twice from user during tokenization leading to fund loss

Handle 0xRajeev Vulnerability details Impact During tokenization of conviction scores, the user can optionally provide FSDs to be locked to let it continue conviction accrual. However, the amount of FSDs specified for locking are debited twice from the user leading to fund loss for user. This, in...

Unrestricted addLiquidity could cause unintended results on front-end apps that listen to events.

Handle shw Vulnerability details Impact The addLiquidity function in Pool.sol lacks an access control, which allows an attacker to add liquidity for any specific user. Front-end apps that listen to AddLiquidity events may be affected by this vulnerability and may go wrong since it is not the user...

Code different from comment

Handle s1m0 Vulnerability details Impact The formula doesn't coincide with the comment. Proof of Concept Tools Used Manual analysis. Recommended Mitigation Steps Correct the code or the comment. --- The text was updated successfully, but these errors were encountered: All reactions...

Missing checks if pairs equal tokens

Email address [email protected] Handle @cmichelio Eth address 0x6823636c2462cfdcD8d33fE53fBCD0EdbE2752ad Vulnerability details The UniswapStyleLib.getAmountsOut, PriceAware.setLiquidationPath and others don't check that path.length + 1 == tokens.length which should always hold true. Also, it does n...

Reliance on unknown governorCheckProposalId is a potential backdoor and risks loss of critical function control

Lines of code Vulnerability details Impact The GuardCM contract is designed to restrict the actions of the Community Multisig CM. However, a potential backdoor could allow the CM to bypass these restrictions and gain unrestricted access. The issue arises from the reliance on a specific proposal I...