Platform: Code4rena
Start Date: 14/06/2022
Pot Size: $100,000 USDC
Total HM: 26
Participants: 59
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 9
Id: 133
League: ETH
Rank: 16/59
Findings: 5
Award: $1,923.99
🌟 Selected for report: 1
🚀 Solo Findings: 0
427.9102 USDC - $427.91
2649.5985 CANTO - $427.91
Functions borrowFresh, repayBorrowFresh, mintFresh, redeemFresh require CNote balance to be strictly zero, reverting unconditionally otherwise. However, as CNote is ERC20 with usual transfer functionality, anyone can send a cNote tokens to the contract itself, effectively disabling all the mentioned operations.
An attacker can borrow a tiny amount of cNote and send a dust amount of it to the CNote contract itself. The attacker can do it, for example, on observing some large repayBorrowFresh() transaction to deny the service at the moment when it is needed to increase the economical damage. Also, there seems to be no special cNote transfer mechanics to burn or transfer to Accountant extra unaccounted cNote to mitigate the attack.
Setting severity to be high as the impact is the freeze of core system functionality as all the main functions require zero CNote balance to operate.
borrowFresh, repayBorrowFresh, mintFresh, redeemFresh require contract balance to be exactly zero. For example, repayBorrowFresh():
//this cannot be a require statement, it must be accounted for if this is the case require(getCashPrior() == 0, "CNote::repayBorrowFresh:Liquidity in Note Lending Market is always flashed"); //make sure that this is remedied
Transfer control mechanics doesn't seem to be updated to the CNote logic:
/** * @notice Checks if the account should be allowed to transfer tokens in the given market * @param cToken The market to verify the transfer against * @param src The account which sources the tokens * @param dst The account which receives the tokens * @param transferTokens The number of cTokens to transfer * @return 0 if the transfer is allowed, otherwise a semi-opaque error code (See ErrorReporter.sol) */ function transferAllowed(address cToken, address src, address dst, uint transferTokens) override external returns (uint) { // Pausing is a very serious situation - we revert to sound the alarms require(!transferGuardianPaused, "transfer is paused"); // Currently the only consideration is whether or not // the src is allowed to redeem this many tokens uint allowed = redeemAllowedInternal(cToken, src, transferTokens); if (allowed != uint(Error.NO_ERROR)) { return allowed; }
function redeemAllowedInternal(address cToken, address redeemer, uint redeemTokens) internal view returns (uint) { if (!markets[cToken].isListed) { return uint(Error.MARKET_NOT_LISTED); } /* If the redeemer is not 'in' the market, then we can bypass the liquidity check */ if (!markets[cToken].accountMembership[redeemer]) { return uint(Error.NO_ERROR); } /* Otherwise, perform a hypothetical liquidity check to guard against shortfall */ (Error err, , uint shortfall) = getHypotheticalAccountLiquidityInternal(redeemer, CToken(cToken), redeemTokens, 0); if (err != Error.NO_ERROR) { return uint(err); } if (shortfall > 0) { return uint(Error.INSUFFICIENT_LIQUIDITY); } return uint(Error.NO_ERROR); }
Consider prohibiting transfer of cNote directly to the token contract in order to keep the cNote accounting intact.
As a more general approach consider performing the balance difference type of control in the CNote functions.
#0 - ecmendenhall
2022-06-21T22:53:36Z
#1 - tkkwon1998
2022-06-22T18:51:04Z
Duplicate of issue #227
2725.9244 CANTO - $440.24
440.2368 USDC - $440.24
Having no reentrancy control and updating the records after external interactions allows for funds draining by reentrancy.
Setting the severity to medium as this is conditional to transfer flow control introduction on future upgrades, but the impact is up to the full loss of the available funds by unrestricted borrowing.
CNote runs doTransferOut before borrowing accounts are updated:
/* * We invoke doTransferOut for the borrower and the borrowAmount. * Note: The cToken must handle variations between ERC-20 and ETH underlying. * On success, the cToken borrowAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ doTransferOut(borrower, borrowAmount); require(getCashPrior() == 0,"CNote::borrowFresh: Error in doTransferOut, impossible Liquidity in LendingMarket"); //Amount minted by Accountant is always flashed from account /* We write the previously calculated values into storage */ accountBorrows[borrower].principal = accountBorrowsNew; accountBorrows[borrower].interestIndex = borrowIndex; totalBorrows = totalBorrowsNew; /* We emit a Borrow event */ emit Borrow(borrower, borrowAmount, accountBorrowsNew, totalBorrowsNew); }
Call sequence here is borrow() -> borrowInternal() -> borrowFresh() -> doTransferOut(), which transfers the token to an external recipient:
/** * @dev Similar to EIP20 transfer, except it handles a False success from `transfer` and returns an explanatory * error code rather than reverting. If caller has not called checked protocol's balance, this may revert due to * insufficient cash held in this contract. If caller has checked protocol's balance prior to this call, and verified * it is >= amount, this should not revert in normal conditions. * * Note: This wrapper safely handles non-standard ERC-20 tokens that do not return a value. * See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ function doTransferOut(address payable to, uint amount) virtual override internal { EIP20NonStandardInterface token = EIP20NonStandardInterface(underlying); token.transfer(to, amount);
There an attacker can call exitMarket() that have no reentrancy control to remove the account of the debt:
/** * @notice Removes asset from sender's account liquidity calculation * @dev Sender must not have an outstanding borrow balance in the asset, * or be providing necessary collateral for an outstanding borrow. * @param cTokenAddress The address of the asset to be removed * @return Whether or not the account successfully exited the market */ function exitMarket(address cTokenAddress) override external returns (uint) {
This attack was carried out several times:
https://certik.medium.com/fei-protocol-incident-analysis-8527440696cc
Consider moving accounting update before funds were sent out, for example as it is done in CToken's borrowFresh():
https://github.com/Plex-Engineer/lending-market/blob/2d423c7c3f62d65182d802deb99cc7bba4e057fd/contracts/CToken.sol#L595-L609 /* * We write the previously calculated values into storage. * Note: Avoid token reentrancy attacks by writing increased borrow before external transfer. `*/ accountBorrows[borrower].principal = accountBorrowsNew; accountBorrows[borrower].interestIndex = borrowIndex; totalBorrows = totalBorrowsNew; /* * We invoke doTransferOut for the borrower and the borrowAmount. * Note: The cToken must handle variations between ERC-20 and ETH underlying. * On success, the cToken borrowAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ doTransferOut(borrower, borrowAmount);
#0 - GalloDaSballo
2022-08-10T22:27:22Z
The warden has shown how, the in-scope codebase has historically been attacked due to a reentrancy attack.
Because:
I agree with Medium Severity
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xf15ers, 0xmint, Bronicle, Dravee, Funen, JMukesh, Limbooo, MadWookie, Picodes, Ruhum, TerrierLover, TomJ, Tutturu, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, cccz, codexploder, cryptphi, csanuragjain, defsec, fatherOfBlocks, gzeon, hake, hansfriese, hyh, ignacio, k, nxrblsrpr, oyc_109, robee, sach1r0, saian, simon135, technicallyty, zzzitron
76.5753 USDC - $76.58
687.9945 CANTO - $111.11
// TODO: This can be optimized a fair bit, but this is safer and simpler for now
uint reservoirBalance_ = token_.balanceOf(address(this)); // TODO: Verify this is a static call
function distributeBorrowerComp(address cToken, address borrower, Exp memory marketBorrowIndex) internal { // TODO: Don't distribute supplier COMP if the user is not in the borrower market.
Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.
assert is used instead of require across the system:
assert(assetIndex < len);
Comptroller:
BaseV1-periphery:
SushiRoll:
UniswapV2Router02:
Using assert in production isn't recommended, consider substituting it with require in all the cases.
event AccountantSet(address accountant, address accountantPrior);
pair.swap(0, amountOut, to, new bytes(0)); // TODO: Add maximum slippage?
#0 - GalloDaSballo
2022-08-02T20:42:53Z
NC
R
Disagree as why would you index an ever changing address
L
1 L 1 R 1 NC