`MultiRewardStaking` does not work with fee-on-transfer tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L115

Vulnerability details

The way MultiRewardStaking handles deposits does not work with fee-on-transfer tokens:

deposit() uses _convertToShares() to compute the number of shares to mint, which returns assets - ie a 1:1 equivalence between shares and the number of asset tokens.

98:   function _convertToShares(uint256 assets, Math.Rounding) internal pure override returns (uint256) {
99:     return assets;
100:   }

But in the case of a fee-on-transfer token, the amount received by the contract will be less than assets. This means the contract has minted more shares than what it should have.

107:   function _deposit(
108:     address caller,
109:     address receiver,
110:     uint256 assets,
111:     uint256 shares
112:   ) internal override accrueRewards(caller, receiver) {
113:     IERC20(asset()).safeTransferFrom(caller, address(this), assets);
114: 
115:     _mint(receiver, shares);

Because withdrawing/redeeming also computes shares and assets on a 1 to 1 basis, users can then call withdraw() and siphon assets from MultiRewardStaking.

121:   function _withdraw(
122:     address caller,
123:     address receiver,
124:     address owner,
125:     uint256 assets,
126:     uint256 shares
127:   ) internal override accrueRewards(caller, receiver) {
128:     if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);
129: 
130:     _burn(owner, shares);
131:     IERC20(asset()).safeTransfer(receiver, assets);
132: 
133:     emit Withdraw(caller, receiver, owner, assets, shares);
134:   }

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Check asset balances of the contract before and after the transfer, and use that amount to _mint() and _burn shares (as the goal is to have a 1:1 ratio between shares and assets anyway). This does introduce a reentrancy vector for ERC777 tokens, so ensure you add appropriate modifiers to prevent attacks.

[c4-judge]

dmvt marked the issue as duplicate of #44

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as nullified