Platform: Code4rena
Start Date: 29/04/2021
Pot Size: $30,000 USDC
Total HM: 3
Participants: 6
Period: 6 days
Judge: cemozer
Total Solo HM: 2
Id: 7
League: ETH
Rank: 1/6
Findings: 5
Award: $18,392.87
🌟 Selected for report: 4
🚀 Solo Findings: 4
🌟 Selected for report: cmichel
14880.9524 BLO - $2,976.19
8928.5714 USDC - $8,928.57
@cmichelio
The UniswapConfig.getTokenConfigBySymbolHash function does not work as getSymbolHashIndex returns 0 if there is no config token for that symbol (uninitialized map value), but the outer function implements the non-existence check with -1.
The same issue occurs also for:
getTokenConfigByCTokengetTokenConfigByUnderlyingWhen encountering a non-existent token config, it will always return the token config of the first index (index 0) which is a valid token config for a completely different token. This leads to wrong oracle prices for the actual token which could in the worst case be used to borrow more tokens at a lower price or borrow more tokens by having a higher collateral value, essentially allowing undercollateralized loans that cannot be liquidated.
Fix the non-existence check.
#0 - ghoul-sol
2021-05-08T19:08:15Z
Duplicate of #24
#1 - ghoul-sol
2021-05-08T21:07:00Z
UniswapConfig has been refactored. Index 0 is considered a non-existent config and all comparison are against that value.
🌟 Selected for report: cmichel
4464.2857 BLO - $892.86
2678.5714 USDC - $2,678.57
@cmichelio
The rewards per market are proportional to their totalBorrows which can be changed by a large holder who deposits lots of collateral, takes out a huge borrow in the market, updates the rewards, and then unwinds the position.
They'll only pay gas fees as the borrow / repay can happen in the same block.
The Comptroller.refreshCompSpeeds function only checks that the single transaction is called from an EOA, but miners (or anyone if a miner offers services like flash bundles for flashbots) can still run flash-loan-like attacks by first sending a borrow tx increasing the totalBorrows, then the refreshCompSpeeds transaction, and then the repay of the borrow, as miners have full control over the transaction order of the block.
The new rate will then persist until the next call to refreshCompSpeeds.
Attackers have an incentive to drive up the rewards in markets they are a large supplier/borrower in. The increased rewards that the attacker receives are essentially stolen from other legitimate users.
Make it an admin-only function or use a time-weighted total borrow system similar to Uniswap's price oracles.
#0 - ghoul-sol
2021-05-08T22:09:43Z
Restricting Comptroller.refreshCompSpeeds function to admin only would centralize an ability to update speeds. I think better solution is a bot that keeps track of markets utilizations and updates speeds when needed. That will also give a way to community to participate.
Also, higher rewards would mean that all participants are getting them and that would bring even more liquidity to the given market and decrease attackers earnings. Attacker could keep moving the liquidity from market to market but everyone would follow quite quickly. If that actually happens, admin has a way to stop the rewards and make refreshCompSpeeds admin-only function as last resolution because comptroller is using proxy pattern.
669.6429 BLO - $133.93
401.7857 USDC - $401.79
@cmichelio
There are several *Verify calls in the caller that lack implementation details.
They are removed in the latest Compound code.
comptroller.transferVerify(address(this), src, dst, tokens);comptroller.mintVerify, comptroller.borrowVerifycomptroller.repayBorrowVerifycomptroller.seizeVerifyThe calls cost gas but do not do anything.
Remove them.
#0 - ghoul-sol
2021-05-05T14:50:51Z
These functions will be left for future implementations.
#1 - cemozerr
2021-05-12T03:59:08Z
🌟 Selected for report: cmichel
1488.0952 BLO - $297.62
892.8571 USDC - $892.86
@cmichelio
The transfer function is used in Maximillion.sol to send ETH to an account.
It is performed with a fixed amount of GAS and might fail if GAS costs change in the future or if a smart contract's fallback function handler is complex.
Consider using the lower-level .call{value: value} instead and checking its success return value.
#0 - ghoul-sol
2021-05-06T16:29:56Z
Maximillion.sol is not being used and will be deleted.
🌟 Selected for report: cmichel
1488.0952 BLO - $297.62
892.8571 USDC - $892.86
@cmichelio
The Comptroller.refreshCompSpeedsInternal function iterates over all markets and does expensive computations like updating all borrower / supply indices.
When the total number of markets is high, this iteration could exceed the total block gas amount breaking the functionality and making it impossible to update the reward distribution speed.
Keep the number of markets low and/or adjust the function to be processable in several transactions.
#0 - ghoul-sol
2021-05-08T17:32:41Z
While true, estimated gas to update speed for 50 markets is 3377184 gas. Current block gas limit is 14,999,986, that means we could in theory, get away with updating as many as 222 markets. This is definitely something to keep in mind along the way, however, in my opinion it's a non-critical issue, low at most.
#1 - cemozerr
2021-05-12T03:11:01Z
I will rate this as low risk, as it won't be an issue until there are many markets, and does not pose a major risk to user funds.