Missing 0 approval

Lines of code Vulnerability details Impact When changing the allowance value from an existing non-zero value, certain tokens e.g., USDT must first be approved by zero before approving the actual allowance. Otherwise the token will not work. Proof of Concept There are two instances of missing zero...

ChainlinkInceptionPriceFeed can report stale price

Lines of code Vulnerability details As stale price is determined by time since last timestamp, the price that is most recent, but wasn't updated for more than PRICEORACLESTALETHRESHOLD say there were no trades on the market will be rejected, which makes system unavailable in such a case. This can...

Unchecked low level calls

Lines of code Vulnerability details Impact The contracts use low level Solidity .call without checking the success value. While these calls should never fail when the contract addresses are correct, we still recommend checking the success return value of these low-level calls. Note: All MIMO and...

[WP-H8] Special ERC721 compatible implementation may allow an attacker to requestLoan without transferring in the NFT collateral

Lines of code Vulnerability details NFT is a fragmented standard, for certain non-standard ERC721 implementations, they may have built-in hooks that can be used to re-enter the contract. Just like ERC777 to ERC20. For example, if the collateral NFT got a pre-transfer hook to the receiver of the...

NFT oracle price request successful or not is not checked

Lines of code Vulnerability details Impact Loan can be destroyed because of no price reported by oracle. or any other reason that makes oracle "work as expected" in bad situations when the get function return success=false Proof of Concept According to the interface of INFTOracle, the first...

An attacker can make users' funds get "locked" in the contract (the owner can get them out and transfer them back to the users)

Lines of code Vulnerability details Impact If a user manages to be the first user to deposit into the contract, he will be minted shares and he can steal all the other users' deposits. Proof of Concept 1. The attacker deposits 1 token into the contract and 1 share is minted to him totalSupply and...

Protocol fees during origination are based on another fee rather than on the loan amount

Lines of code Vulnerability details Impact Protocol fee revenue will be much lower than expected. If the average NFT loaned on the platform is worth $900, openFeeShare will be $9, and protocolFeeShare will be only $0.90. There would have to be more than 50,000 such loans to cover the cost of this...

Yield source does not correctly calculate share conversions

Lines of code Vulnerability details The aTokens’ value is pegged to the value of the corresponding supplied asset at a 1:1 ratio and can be safely stored, transferred or traded. All yield collected by the aTokens' reserves are distributed to aToken holders directly by continuously increasing thei...

Owner of the PoolAddressesProviderRegistry Contract Can Update the Pool Address and Effectively Lock Deposited Funds by Preventing All Withdrawals

Lines of code Vulnerability details Impact The owner of the PoolAddressesProviderRegistry contract is able to register and unregister providers as they see fit. Because AaveV3YieldSource.sol dynamically queries the Aave pool through this contract, it is possible for the owner of this Aave contrac...

Lower LVT is treated as less restrictive, while it's vice versa

Lines of code Vulnerability details Impact Lender can accept overly restrictive LTV the lowest possible at the moment, with high enough probability being able to seize the collateral after a short time. Lender can set ltvBPS to zero with and immediately liquidate with removeCollateral any loan no...

Owner or Managers can rug Aave rewards

Lines of code Vulnerability details Impact A malicious owner or manager can steal all Aave rewards that are meant for PoolTogether users Even if the user is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. Proof of Concept File:...

NFTPairWithOracle's _lend ignores accepted.oracle and allows to start loan with empty params.oracle

Lines of code Vulnerability details Impact As lend doesn't require params.oracle to be valid, while removeCollateral does, the loan initiation with an empty oracle can lead to ignoring collateral valuation. As the deals are OTC this can be seen as lender decision. However, lend ignores...

Reentrency in claimRewards in ConcurRewardPool

Judge @GalloDaSballo has assessed the 1st item in QA Report 163 as Medium risk. The relevant finding follows: … Reentrency in claimRewards in ConcurRewardPool The function claimRewards is open to reenterncy, if the safeTransfer function of a token calls the claimRewards again the tokens can be...

Potential Sandwich Attack: Arbitrage bots can front run reward tokens being sent to the liquidity mining contracts

Lines of code Vulnerability details Impact For the PARMiner and DemandMiner contracts, arbitrage bots could harvest significant portion of rewards by monitoring MEV, and front run any reward token either a.mimo or par being transferred to the liquidityMining contract i.e. call the deposit functio...

Potential reentrance in claimRewards

Judge @GalloDaSballo has assessed the 1st item in QA Report 36 as Medium risk. The relevant finding follows: … POC IERC20tokensi.safeTransfermsg.sender, getting; rewardmsg.sendertokensi = 0; Considering there are exterTokens, it is possible that some token will provide reentry opportunities...

Customers cannot redeem() LP tokens to non-EOA accounts

Lines of code Vulnerability details The use of payable.transfer is heavily frowned upon because it can lead to the locking of funds. The transfer call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfe...

Unsafe ERC20 transfer Operations

Findings Unsafe ERC20 transfer Operations The transfer and transferFrom functions return a Boolean value which should be checked for successful transfer, Some tokens do not revert if the transfer did fail but return false. the protocol Joyn do have some lines for code that make a use of these...

Index mint and burn calls can be front run

Lines of code Vulnerability details Impact Both in the mint and burn cases all the user supplied / due to a user assets can be stolen by an attacker, who detects correspondingly asset transfer calls / Index token transfer call and front runs Index contract's mint / burn call with own address as a...

Missing Validations for the return values of Chainlink Price feeds

Lines of code Vulnerability details Impact You check only the answerThe price after calling the chainlink Chainlink Price feeds in the following lines. In addition, you need to check whether the data is really updated. Proof of Concept Tools Used code review Recommended Mitigation Steps Please ad...

loss of funds if there is no discount

Lines of code Vulnerability details citadelAmount is initialized to 0 by default, and then if the discount is greater than zero, it is set to the amount out. But if the discount is 0, it is not set to the amount out and citadelAmount stays 0 and then divided by assetDecimalsNormalizationValue, bu...

admin can rug

Lines of code Vulnerability details the mint function in CitadelToken requires the role CITADELMINTERROLE. this role is managed by the governance: setRoleAdminCITADELMINTERROLE, CONTRACTGOVERNANCEROLE; therefore the admin can mint to himself an unlimited amount. --- The text was updated...

StakedCitadelVester/claimableBalance() has problems when users keep vesting.

Lines of code Vulnerability details Impact When users vest in StakedCitadelVester, the tokens will be locked in vest duration. Users can call claim to get back their tokens. The claimable amount is calculated in claimableBalance. Before the duration ends, the claimable amount is locked...

getPricePerFullShare() can be much lower than expected

Lines of code Vulnerability details Impact Since balance does not include the amount 95% that's been transferred to the strategy, the getPricePerFullShare may only give 5% of the intended price. getPricePerFullShare is used in other contracts, such as Funding.sol and may also be used in other...

Stale price used when citadelPriceFlag is cleared

Lines of code Vulnerability details During the video it was explained that the policy operations team was meant to be a nimble group that could change protocol values considered to be safe. Further, it was explained that since pricing comes from an oracle, and there would have to be unusual...

Chainlink's latestRoundData missing security checks

Lines of code Vulnerability details Impact Protocol uses Chainlink as one of the oracles that provides prices for the assets. Chainlink's latestRoundData is used but the implementation is missing important security checks that can result in stale and incorrect prices being returned. Proof of...

Funding.deposit() doesn't work if there is no discount set

Lines of code Vulnerability details Impact The Funding contract's deposit function uses the getAmountOut function to determine how many citadel tokens the user should receive for their deposit. But, if no discount is set, the function always returns 0. Now the deposit function tries to deposit 0...

Race between governance and strategist on other token earned

Lines of code Vulnerability details Impact There is a race between the strategist and the governance to report other tokens earned by the strategy. Indeed the strategist can trigger the function 1 by calling the strategy while the governance can call 2. Both these functions can report earn tokens...

If there is no discount then buying citadel in funding will always return 0 citadel

Lines of code Vulnerability details Impact Users are unable to buy citadel in funding if the discount is 0. Proof of Concept In the funding contract if the discount is 0 then getAmountOut will always return 0 and users won't be able to use funding to buy citadel. If discount is 0 then the if is...

Should check return data from chainlink aggregators

Lines of code Vulnerability details Impact The refreshedAssetPerBaseInUQ function in the contract ChainlinkPriceOracle.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices. The...

Use of deprecated Chainlink function latestAnswer

Lines of code Vulnerability details function normalizeAggregatorAnswerIAggregatorV3Interface aggregator internal view returns uint256 int256 answer = aggregator.latestAnswer; uint8 decimals = aggregator.decimals; requireanswer 0, "invalidoracleanswer"; //converts the answer to have 18 decimals...

Division before Multiplication May Result In No Interest Being Accrued

Lines of code Vulnerability details Impact There is a division before multiplication bug in NFTVault.calculateAdditionalInterest which may result in no interesting being accrued and will have significant rounding issues for tokens with small decimal places. This issue occurs since an intermediate...

Wrong lockFor can lost tokens

Lines of code Vulnerability details Impact If the owner incorrectly uses the lockFor method and calls it twice with the same nftIndex, it will overwrite the record and the deposited jpeg can never be retrieved. Proof of Concept Owner call lockForAlice,0,100; Owner call lockForBob,0,100; 100 token...

Low level call returns true if the address doesn't exist

Lines of code Vulnerability details Impact As written in the solidity documentation, the low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior t...

Transferring admins does not work for CToken

Lines of code Vulnerability details Impact The CToken implements an acceptAdmin function that sets the new admin to the pendingAdmin. But CToken does not implement a setPendingAdmin function to set the pendingAdmin in the first place. Therefore, the acceptAdmin function is useless and CTokens can...

Using transferFrom on ERC721 tokens

Lines of code Vulnerability details In the function closeLoan of contract NFTLoanFacilitator.sol, the transferFrom keyword is used instead of safeTransferFrom. If the arbitrary sendCollateralTo address is a contract and is not aware of the incoming ERC721 token, the sent token could be locked. I...

currentLoanOwner can manipulate loanInfo when any lenders try to buyout

Lines of code Vulnerability details Impact If an attacker already calls lend to lend to a loan, the attacker can manipulate loanInfo by reentrancy attack when any lenders try to buyout. The attacker can set bad values of lendInfo e.g. very long duration, and 0 interest rate that the lender who...

Missing balance check before permitting

Lines of code Vulnerability details Impact there is no owner balance checking before permit amount to spender. So owner can permit more than its balance to spender. Maybe this issue is not very risky but checking owner balance before permitting is more safe Proof of Concept Provide direct links t...

Lender can lose funds

Lines of code Vulnerability details Impact Since some tokens take transfer fees on performing transfer operations and current contract implementation is not considering same, lender funds could be lost Proof of Concept 1. Attacker creates a loan request for token XYZ taking 10% transfer fees 2...

Chain ID Is Not Resistant To Hard Fork and Other Token Supports In The Oracle Contract

Lines of code Vulnerability details Impact During the code review, It has been observed only the following chain ids are supported for the chainlink. 1 and 42 - The contracts are not upgradeable therefore If there is any hard fork or new chain support, the contract should be deployed again with...

Royalties do Not Support Fee-On-Transfer Tokens

Lines of code Vulnerability details Impact The RoyaltyVault.sol contract interacts with the Splitter.sol to send accumulated royalties to the collection's respective recipients. The sendToSplitter function will query the balance of the royalty asset and send the amount after fee deductions to the...

expiry is not work in contract

Reported by warden rayn in 98, duplicate of 28 Low Risk Findings - expiry is not work in contract In document, we define that: Expiry The expiry date of the market. If a market has not settled by its expiry date, it will automatically settle at the lower bound of its Valuation Range. But in...

Function getUserPastLock may return incorrect values

Lines of code Vulnerability details Impact userLocks array can contain elements with the same fromBlock properties, but different total locked amounts. This edge case is not considered in the implementation of getUserPastLock function, which returns a value as soon as it finds a UserLock with...

System could be wrapped and made useless without contract whitelisting

Lines of code Vulnerability details Impact Anyone could create a contract or a contract factory "PAL Locker" with a fonction to deposit PAL tokens through a contract, lock them and delegate the voting power to the contract owner. Then, the ownership of this contract could be sold. By doing so,...

UserLock information can be found during emergency mode

Lines of code Vulnerability details When the contract is in blocked state emergency mode, the protocol wants to return an empty UserLock info, on calling the function getUserLock. However, there is another way, by which the users can find the same information. The below function is not protected...

Add a timelock to PaladinRewardReserve functions

Lines of code Vulnerability details Impact The owner of PaladinRewardReserve can approve and transfer any amount of tokens with no limits on any account. This is not good for investors. To give more trust to users: these functions should be put behind a timelock. Proof of Concept Tools Used VS Co...

User can always stay in UNSTAKE_PERIOD

Lines of code Vulnerability details Impact Due to how the cooldown period is calculated after a transfer, a user can strategically transfer between accounts to increase their cooldown timestamp while keeping it within the UNSTAKEPERIOD, so they can unstake anytime, defeating the cooldown mechanis...

DropPerSecond is not updated homogeneously, the rewards emission can be much higher than expected in some cases

Lines of code Vulnerability details function updateDropPerSecond internal returns uint256 // If no more need for monthly updates = decrease duration is over ifblock.timestamp startDropTimestamp + dropDecreaseDuration // Set the current DropPerSecond as the end value // Plus allows to be updated i...

Possible problem when mintToken and the user wants to purchase a token

Lines of code Vulnerability details Impact A User can mint tokens even if the payment doesn't succeed. Proof of Concept the contract ERC721Payable uses erc20 to handle payments. When this contract handles the payment of the user , calls the function transferFrom from the ERC20 payableToken variab...

RoyaltyVault.sol is Not Equipped to Handle On-Chain Royalties From Secondary Sales

Lines of code Vulnerability details Impact The Joyn documentation mentions that Joyn royalty vaults should be equipped to handle revenue generated on a collection's primary and secondary sales. Currently, CoreCollection.sol allows the collection owner to receive a fee on each token mint, however,...

Differing percentage denominators causes confusion and potentially brick claims

Lines of code Vulnerability details Details & Impact There is a PERCENTAGESCALE = 10e5 defined, but the actual denominator used is 10000. This is aggravated by the following factors: 1. Split contracts are created by collection owners, not the factory owner. Hence, there is a likelihood for someo...