bunker.finance contest - leastwood's results

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CEther.sol#L165-L168

Vulnerability details

Impact

The .transfer() function intends to transfer an ETH amount with a fixed amount of 2300 gas. This function is not equipped to handle changes in the underlying .send() and .transfer() functions which may supply different amounts of gas in the future. Additionally, if the recipient implements a fallback function containing some sort of logic, this may inevitably revert, meaning the original supplier cannot redeem their cTokens.

Recommended Mitigation Steps

Consider using .call() instead with the checks-effects-interactions pattern implemented correctly. Careful consideration needs to be made to prevent reentrancy.

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L240-L242 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L260-L262 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L469-L472 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L496-L499 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L1139-L1155 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L1222-L1243

Vulnerability details

Impact

The updateCompSupplyIndex() and distributeSupplierComp() functions are used by Compound to track distributions owed to users for supplying funds to the protocol. Bunker protocol is a fork of compound with NFT integration, however, part of the original functionality appears to have been mistakenly commented out. As a result, whenever users enter or exit the protocol, COMP distributions will not be correctly calculated for suppliers. At first glance, its possible that this was intended, however, there is nothing stated in the docs that seems to indicate such. Additionally, the COMP distribution functionality has not been commented out for borrowers. Therefore, tokens will still be distributed for borrowers.

Both the updateCompSupplyIndex() and updateCompBorrowIndex() functions operate on the same compSpeeds value which dictates how many tokens are distributed on each block. Therefore, you cannot directly disable the functionality of supplier distributions without altering how distributions are calculated for borrowers. Because of this, suppliers can manipulate their yield by supplying tokens, calling updateCompSupplyIndex() and distributeSupplierComp(), removing their tokens and repeating the same process on other accounts. This completely breaks all yield distributions and there is currently no way to upgrade the contracts to alter the contract's behaviour. Tokens can be claimed by redepositing in a previously "checkpointed" account, calling claimComp() and removing tokens before re-supplying on another account.

Recommended Mitigation Steps

Consider commenting all behaviour associated with token distributions if token distributions are not meant to be supported. Otherwise, it is worthwhile uncommenting all occurrences of the updateCompSupplyIndex() and distributeSupplierComp() functions.

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L23-L48 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L126-L144 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L170-L178

Vulnerability details

Impact

The Uniswap oracle utilises a 30 minute TWAP window which helps to protect the protocol from flash loan manipulation. However, in order to keep this data fresh, the update() function must be called by some individual on each 30 minute interval. Failure to do so will result in an extended TWAP window which may return a drastically different price when compared to the 30 minute TWAP window. As a result, if the Uniswap oracle is not updated for an extended period of time and a number of individuals borrow based on this information, a subsequent update may lead to a liquidation as the 30 minute TWAP price is lower than the currently referenced price.

The sponsor has stated that they will be in charge of constantly upkeeping this oracle, however, this is a centralised point of failure and there is no guarantee or incentive that this will continue in the long run. I strongly believe that protocols need to be self-sufficient and they should not be reliant on some benevolent actor.

Recommended Mitigation Steps

Consider integrating some sort of incentive behaviour into the Uniswap oracle model to ensure price information is updated on each update interval. There are a couple of implementations which verify block data on-chain, ensuring past price information is valid, however, this has serious overhead gas costs due to on-chain verification. Therefore, I believe some sort of on-chain incentive mechanism is ideal in this situation.