search

code-423n4 / 2022-04-mimo-findings

0 stars 0 forks source link

Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies #156

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/AdminInceptionVault.sol#L81 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/AdminInceptionVault.sol#L151 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/DemandMinerV2.sol#L67 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L129 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L274 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L290

Vulnerability details

Impact

Wrong amount allowed / deposited / accounted

Proof of Concept

core/contracts/inception/AdminInceptionVault.sol:
   81:     asset.safeTransferFrom(msg.sender, address(this), _depositAmount);
  151:     asset.safeTransferFrom(msg.sender, address(this), _amount);

core/contracts/liquidityMining/v2/DemandMinerV2.sol:
  67:     _token.safeTransferFrom(msg.sender, address(this), amount);

supervaults/contracts/SuperVault.sol:
  129:     IERC20(asset).transferFrom(msg.sender, address(this), depositAmount);
  274:     token.transferFrom(msg.sender, address(this), amount);
  290:     token.transferFrom(msg.sender, address(this), depositAmount);

Arbitrary ERC20 tokens can be passed With a transfer, the received amount should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

m19 commented 2 years ago

While technically true, we do not think this is a medium risk issue:

gzeoneth commented 2 years ago

I agree with the sponsor there doesn't seems to be a pathway for a fee-on-transfer token to not revert in every instance of transfer except for the one in DemandMinerV2, which in the doc stated:

Liquidity Mining V2 The next version of reward contracts for the users of the protocol. In the first iteration of liquidity mining contracts, users earned MIMO tokens on their debt and staked LP and PAR tokens.

where the LP and PAR tokens are not fee-on-transfer. Downgrading this to Low / QA.

gzeoneth commented 2 years ago

Considered with #153