Actual yield source check on address will succeed for non-existent contract

Handle 0xRajeev Vulnerability details Impact Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent per EVM design. Solidity documentation warns: "The low-level functions call, delegatecall and staticcall return true as their first return value if the...

safe transfer

Handle gpersoon Vulnerability details Impact 3 of the 5 yield sources use safeTransfer/safeTransferFrom ATokenYieldSource.sol, IdleYieldSource.sol, YearnV2YieldSource.sol However 2 of the 5 yielsd source use transfer/transferFrom and also don't check the result of the functions...

Return values of ERC20 transfer and transferFrom are unchecked

Handle shw Vulnerability details Impact In the contract RCTreasury, the return values of ERC20 transfer and transferFrom are not checked, which could be false if the transferred token is not ERC20-compliant. In that case, the transfer fails without being noticed by the calling contract. Proof of...

Market opening time can be before the locking time

Handle shw Vulnerability details Impact In the function createMarket of RCFactory, the market opening time is not checked before the market locking time, and thus markets can be created with invalid time configurations accidentally. Besides, if the advancedWarning is non-zero, there are two check...

usage of safeApprove

Handle pauliax Vulnerability details Impact depositInVault in contract YearnV2YieldSource calls safeApprove when the allowance is less than the token balance: if token.allowanceaddressthis, addressv token.balanceOfaddressthis token.safeApproveaddressv, typeuint256.max; This does not mean that the...

Function foreclosureTimeUser returns a shorter user's foreclosure time than expected

Handle shw Vulnerability details Impact The function foreclosureTimeUser of RCTreasury underestimates the user's foreclosure time if the current time is not the user's last rent calculation time. The underestimation of the foreclosure time could cause wrong results when determining the new owner ...

A malicious actor can let market sponsors send sponsorship to unintended markets

Handle shw Vulnerability details Impact The function sponsoraddress sponsorAddress, uint256 amount in RCMarket is permissionless, which allows a malicious actor to sponsor any market in the name of some sponsor as long as enough tokens are approved. The malicious actor could then add sponsorship ...

Problems with non-standard compliant ERC20 tokens

Handle cmichel Vulnerability details Vulnerability Details Some tokens like USDT don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert...

Card affiliate payouts are skipped if a single card does not have an affiliate

Handle cmichel Vulnerability details Vulnerability Details The Market.initialize function sets the cardAffiliateCut to zero if a single cardAffiliateAddresses is the zero address. for uint256 i = 0; i numberOfCards; i++ if cardAffiliateAddressesi == address0 cardAffiliateCut = 0; Impact Even if a...

Missing call to removeOldBids may affect foreclosure

Handle 0xRajeev Vulnerability details Impact Orderbook.removeBids as commented “///remove bids in closed markets for a given user ///this can reduce the users bidRate and chance to foreclose” removeOldBids is performed currently in Market.newRental and Treasury.deposit to “do some cleaning up, it...

Missing threshold check on critical protection mechanism minRentalDayDivisor

Handle 0xRajeev Vulnerability details Impact Minimum rental duration is acknowledged as one of the two critical protection mechanisms for the market functioning. The setMinRental is called from the constructor with 246 which sets the minimum duration to 10 minutes. However, a threshold check is...

User deposits can be turned into sponsors and then be stolen

Handle cmichel Vulnerability details Vulnerability Details When a user deposits to the treasury they first approve the contract and then call its deposit action which performs an ERC20.transferFrom. It's possible for an attacker to frontrun the final deposit transaction after the user approval an...

Wrong calculation on _collectRentAction

Handle adelamo Vulnerability details Impact The method collectRentAction contains the following code: ... else if !foreclosed && limitHit && marketLocked // CASE 4 // didn't foreclose AND // did hit time limit AND // did lock market // THEN refund rent between the earliest event and now if...

Missing checkOnERC721Received deviates from ERC721 and could lock/lose NFTs

Handle 0xRajeev Vulnerability details Impact ERC721 specification for safeTransferFrom says: “this function checks if to is a smart contract code size 0. If so, it calls onERC721Received on to and throws if the return value is not bytes4keccak256“onERC721Receivedaddress,address,uint256,bytes”.”...

Market-specific pause is not checked for payout

Handle cmichel Vulnerability details Vulnerability Details The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.payout. A paused market contract can therefore still pay out using payArtist, payCardAffiliate, payMarketCreator,...

Market-specific pause is not checked for payRent

Handle cmichel Vulnerability details Vulnerability Details The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.payRent. The market contract can therefore still collect rent even when it's paused using Market.updateTimeHeldLimit,...

NFT Hub implementation deviates from ERC721 for transfer functions

Handle 0xRajeev Vulnerability details Impact ERC721 standard and implementation allows the use of approved addresses to affect transfers besides the token owners. However, the L2 NFT Hub implementation deviates from ERC721 by ignoring the presence of any approvers in the overriding function...

Flows can bypass market and global pause

Handle 0xRajeev Vulnerability details Impact Ability to pause all token transfers and all state changes for contracts is a “guarded-launch” best-practice for emergency situations for newly launched projects. The project implements this using a marketsPaused flag per market and a globalPause flag...

function topupMarketBalance should have a balancedBooks modifier

Handle pauliax Vulnerability details Impact I expect every function in the Treasury that interacts with erc20 tokens to have this balancedBooks modifier to make sure that "funds haven't gone missing during this function call". To make sure that tokens were indeed transferred and marketBalance...

Use SafeERC20/TransferHelper on RCTreasury

Handle adelamo Vulnerability details Impact Even though the uberOwner controls what ERC20 to use on RCTreasury, it is highly recommendable to use TransferHelper or SafeERC20 in order to interact safely with the ERC20. Tools Used Editor --- The text was updated successfully, but these errors were...

Deposits can be denied by abusing maxContractBalance

Handle cmichel Vulnerability details Vulnerability Details The treasury implements a max contract balance check in the deposit function: require erc20.balanceOfaddressthis + amount This is not only restricted to whales, miners/users can do the same using same-block cross-transaction flashloans an...

Critical uberOwner address changes should be a two-step process

Handle 0xRajeev Vulnerability details Impact As specified, uberOwners of Factory, Orderbook and Treasury have the highest privileges in the system because they can upgrade contracts of market, Nfthub, order book, treasury, token and factory which form the critical components of the protocol. The...

Missing call to removeUserFromOrderbook after user is foreclosed

Handle 0xRajeev Vulnerability details Impact Orderbook’s removeUserFromOrderbook is used to delete/remove user’s bids when they are deemed foreclosed. This is called in Market newRental and Treasury withdrawDeposit when users are determined to be foreclosed given their deposit and bid situation...

Anyone can affect deposits of any user and turn the owner of the token

Handle adelamo Vulnerability details Impact On RCTreasury, we have the method collectRentUser. This method is public, so anyone can call it using whatever user and whatever timestamp. So, calling this method using user = XXXXX and timeToCollectTo = typeuint256.max, would make isForecloseduser =...

Deposit whitelist enforced on msg.sender instead of user

Handle 0xRajeev Vulnerability details Impact The Treasury deposit function credits amount to the user address in parameter instead of the msgSender that is actually making the deposit whose rationale as explained in the Natspec comment is that this may be called via contract or L1-L2 bot. However...

Check that marketLockingTime >= marketOpeningTime

Handle pauliax Vulnerability details Impact There is no check that timestamps1 marketLockingTime = timestamps0 marketOpeningTime. I think that should be enforced just in case to prevent market locking before opening. Recommended Mitigation Steps Add in Factory createMarket: require timestamps1 =...

maxSumOfPrices check is broken

Handle 0xRajeev Vulnerability details Impact rentAllCards requires the sender to specify a maxSumOfPrices parameter which specifies “limit to the sum of the bids to place” as specified in the Natspec @param comment. This is apparently for front-run protection. However, this function parameter...

Missing Factory-only access check leads to loss of funds

Handle 0xRajeev Vulnerability details Impact The Market sponsoraddress sponsorAddress, uint256 amount function is an externally callable function that is specified to be callable only by the Factory contract during market creation, as documented in the Natspec @dev comment: "called by Factory...

Deposits don't work with fee-on transfer tokens

Handle cmichel Vulnerability details 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. Impact The deposit function will introduce...

RCNftHubL2.safeTransferFrom not accoring to spec

Handle cmichel Vulnerability details Vulnerability Details The RCNftHubL2.safeTransferFrom function does not correctly implement the ERC721 spec: When using safeTransferFrom, the token contract checks to see that the receiver is an IERC721Receiver, which implies that it knows how to handle ERC721...

Malicious user can trigger another user’s removal

Handle 0xRajeev Vulnerability details Impact By allowing anyone to call removeUserFromOrderbook instead of only Market::newRental or Treasury::withdrawDeposit or collectRentUser which may result in foreclosures and hence may need to trigger user removal, a malicious user can trigger another's...

Unchecked return value from ERC20

Handle cmichel Vulnerability details Vulnerability Details The ERC20.transfer and ERC20.transferFrom functions return a boolean value indicating success. This parameter needs to be checked for success. According to the standard the return value must be checked for true, otherwise the transfer wil...

User could deposit for free

Handle s1m0 Vulnerability details Impact deposit function doesn't check the return value of transferFrom that means if the erc20 token return false instead of reverting the user could deposit for free. Tools Used Manual analysis Recommended Mitigation Steps Use openzeppelin's SafeERC20 library. -...

Missing balancedBooks modifier could result in failed system insolvency detection

Handle 0xRajeev Vulnerability details Impact The balancedBooks modifier is used to “check that funds haven't gone missing during this function call” and is applied to deposit, withdrawDeposit, payRent, payout and sponsor Treasury functions which move funds in and out of the Treasury or adjust its...

Market-specific pause is not checked for sponsor

Handle cmichel Vulnerability details Vulnerability Details The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.sponsor. A paused market contract can therefore still deposit as a sponsor using Market.sponsor Impact The market-specif...

Pot distribution does not need to add up to 100%

Handle cmichel Vulnerability details Vulnerability Details The Factory.setPotDistribution allows specifying values that add up to less than 100% because of the inequality = 1000 instead of an equality == 1000. Impact If using less than 100%, funds could become stuck in the market for certain mode...

minRentalDayDivisor may change

Handle pauliax Vulnerability details Impact Market initializes minRentalDayDivisor from the treasury but this variable can be later changed in treasury function setMinRental. Here I am not sure if the market still needs to use old cached version of minRentalDayDivisor as all the interactions with...

Unused return value from erc20.transfer()/ erc20.transferFrom()

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 In RcTreasury.sol Tools Used manual review Recommended...

anyone can call function sponsor

Handle pauliax Vulnerability details Impact This function sponsor should only be called by the factory, however, it does not have any auth checks, so that means anyone can call it with an arbitrary sponsorAddress address and transfer tokens from them if the allowance is 0: /// @notice ability to...

transferFrom result not checked

Handle gpersoon Vulnerability details Impact The function deposit of SafeERC20.sol relies on the fact that transferFrom will revert if it can't transfer the erc20 tokens. However, depending on the ERC20 token, this doesn't happen and you have to check the result of transferFrom. With the wrong...

erc20 transfer and transferFrom functions

Handle pauliax Vulnerability details Impact When transfering erc20 tokens, functions transfer and transferFrom are used. These functions return boolean to indicate if the action was successful, however, none of the usages check the returned value: erc20.transferFrommsgSender, addressthis, amount;...

Can access cards of other markets

Handle gpersoon Vulnerability details Impact Within RCMarket.sol the functions ownerOf and onlyTokenOwner do not check if the cardId/token is smaller than numberOfCards. So it's possible to supply a larger number and access cards of other markets. The most problematic seems to be upgradeCard. Her...

Unchecked ERC20 transfers can cause lock up

Handle axic Vulnerability details Impact Some major tokens went live before ERC20 was finalised, resulting in a discrepancy whether the transfer functions a should return a boolean or b revert/fail on error. The current best practice is that they should revert, but return “true” on success...

Users can avoid paying borrowing interest after the fyToken matures

Handle shw Vulnerability details Impact According to the protocol design, users have to pay borrowing interest when repaying the debt with underlying tokens after maturity. However, a user can give his vault to Witch and then buy all his collateral using underlying tokens to avoid paying the...

User can redeem more tokens by artificially increasing the chi accrual

Handle shw Vulnerability details Impact A user can artificially increase the chi accrual after maturity by flash borrow on Compound, which affects the exchange rate used by the chi oracle. As a result, the user redeems more underlying tokens with the same amount of fyTokens since the accrual is...

Possible DoS attack when creating Joins in Wand

Handle shw Vulnerability details Impact It is possible for an attacker to intendedly create a fake Join corresponding to a specific token beforehand to make Wand unable to deploy the actual Join, causing a DoS attack. Proof of Concept The address of Join corresponding to an underlying asset is...

Potential griefing with DoS by front-running vault creation with same vaultID

Handle 0xRajeev Vulnerability details Impact The vaultID for a new vault being built is required to be specified by the user building a vault via the build function instead of being assigned by the Cauldron/protocol. An attacker can observe a build as part of a batch transaction in the mempool,...

buyFYToken and buyBase do not reimburse leftovers

Handle pauliax Vulnerability details Impact functions buyFYToken and buyBase do not reimburse leftovers. It checks that the transferred amount is between min and max boundaries, however, it does not send back any excess amount back to the sender nor it accounts it in the update function e.g. it...

Uninitialized or Incorrectly set auctionInterval may lead to liquidation engine livelock

Handle 0xRajeev Vulnerability details Impact The grab function in Cauldron is used by the Witch or other liquidation engines to grab vaults that are under-collateralized. To prevent re-grabbing without sufficient time for auctioning collateral/debt, the logic uses an auctionInterval threshold to...

Violation of implicit constraints in batched operations may break protocol assumptions

Handle 0xRajeev Vulnerability details Impact The Ladle batching of operations is a complex task as noted by the project lead which has implicit constraints on what operations can be bundled together in a batch, which operations can/have-to appear how many times and in what order/sequence etc. Som...