Consistently check account balance before and after transfers for Fee-On-Transfer discrepencies

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L149-L159
https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L548-L562
https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L611-L613

Vulnerability details

Impact

Wrong increase on margin amount.
Wrong amount emitted.

Proof of Concept

In the public function addMarginFor(), an arbitrary ERC20 token can be passed as an argument:

File: MarginAccount.sol
149:     function addMarginFor(uint idx, uint amount, address to) override public whenNotPaused {
...
155:             supportedCollateral[idx].token.safeTransferFrom(_msgSender(), address(this), amount); //@audit FOT? whitelistCollateral() onlyGovernance > _addCollateral() > here, arbitrary tokens possible.
156:         }
157:         margin[idx][to] += amount.toInt256();
158:         emit MarginAdded(to, idx, amount, _blockTimestamp());
159:     }

548:     function _addCollateral(address _coin, uint _weight) internal {
...
555:         supportedCollateral.push(
556:             Collateral({
557:                 token: IERC20(_coin), //@audit arbitrary coin
558:                 weight: _weight,
559:                 decimals: ERC20Detailed(_coin).decimals() // will fail if .decimals() is not defined on the contract
560:             })

611:     function whitelistCollateral(address _coin, uint _weight) external onlyGovernance {
612:         _addCollateral(_coin, _weight); //@audit arbitrary coin
613:     }

Even if the whitelisted Collateral coin is known by the Governance: the received amount should be calculated every time to take into consideration a possible present or future transfer-on-fee or deflation.
Also, it's a good practice for the future of the solution.

Tools Used

VS Code

Recommended Mitigation Steps

Check the balance before and after the transfer to take into account the Fees-On-Transfer.
As an example:

        uint256 balanceBefore = supportedCollateral[idx].token.balanceOf(address(this)); //@audit-info new code
...
        } else {
            supportedCollateral[idx].token.safeTransferFrom(_msgSender(), address(this), amount); //@audit FOT accounting
        }
        uint256 receivedAmount = supportedCollateral[idx].token.balanceOf(address(this)) - balanceBefore; //@audit-info new code
        margin[idx][to] += receivedAmount.toInt256(); //@audit-info corrected code
        emit MarginAdded(to, idx, receivedAmount, _blockTimestamp()); //@audit-info corrected code

[atvanguard]

It's a design choice to not support deflationary tokens at all. The vetting process will happen off-chain.

[JasoonS]

It is something to keep in mind, not something to be changed in the contracts.

Setting to low risk.

Something that should be documented and aware of for sure! Some kind of rubric that tokens must pass before being accepted.

[JeeberC4]

Grouping with warden's QA report #94