_amount requires to be updated to contract balance increase

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/AdminInceptionVault.sol#L149-L154

Vulnerability details

Impact

Every time transferFrom or transfer function in ERC20 standard is called there is a possibility that underlying smart contract did not transfer the exact amount entered.
It is required to find out contract balance increase/decrease after the transfer.
This pattern also prevents from re-entrancy attack vector.

Proof of Concept

Tools Used

Recommended Mitigation Steps

Recommended code:
function deposit(address _collateralType, uint256 _amount) public override onlyOwner {
IERC20 asset = IERC20(_collateralType);

uint256 balanceBefore = asset.balanceOf(address(this)); // remembering asset balance before the transfer
asset.safeTransferFrom(msg.sender, address(this), _amount);
_amount = asset.balanceOf(address(this)) - balanceBefore; // updating actual _amount to the contract balance increase

asset.safeIncreaseAllowance(address(_a.core()), _amount);
_a.core().deposit(_collateralType, _amount);

}

[m19]

Duplicate of #156

[gzeoneth]

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]

Consider with warden's' QA report #51