search

code-423n4 / 2022-05-bunker-findings

1 stars 0 forks source link

`COMP` Distributions Can Be Manipulated And Duplicated Across Any Number Of Accounts #105

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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.

bunkerfinance-dev commented 2 years ago

We are not going to use the COMP code. We could fix documentation or comment more code to make this clearer though.

gzeoneth commented 2 years ago

Comptroller.sol is in scope of this contest, and there are no indication that token distribution will be disabled despite the sponsor claim they are not going to use the $COMP code. However, it is also true the deployment setup within contest repo lack the deployment of $COMP and its distribution. I believe this is a valid Med Risk issue given fund(reward token) can be lost in certain assumptions.