code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L120-L124 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206

Vulnerability details

Some ERC20 tokens, such as Tether (USDT), allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance.

Impact

The current implementation does not work properly with fee-on-transfer tokens when depositing/withdrawing the tokens, which will lead to either the calls reverting or some users being able to claim more than they should be, with the final users trying to withdraw from the lockbox being stuck with all of the fees.

Proof of Concept

When a token is transferred in the amount is stored, and this amount may be wrong:

    function _saveTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private {
        transferredAmount[schainHash][erc20Token] += amount;
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L299-L301

Later when the user asks for it back they're able to ask for the original amount even though the contract doesn't hold that amount

        _removeTransferredAmount(schainHash, message.token, message.amount);
        require(
            ERC20Upgradeable(message.token).transfer(message.receiver, message.amount),

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L158-L160

    function _removeTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private {
        transferredAmount[schainHash][erc20Token] -= amount;
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308

If other users have deposited without withdrawing then the user will be able to withdraw more than they should be able to. The post-kill admin withdrawal has the same issue:

            ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L206

Tools Used

Code inspection

Recommended Mitigation Steps

One way to work around the issue is to measure the contract's balance right before and after the asset-transferring functions, and using the difference rather than the stated transferred amount.

cstrangedk commented 2 years ago

Duplicate and disputed of #42, #53, #20

GalloDaSballo commented 2 years ago

Dup of #50

code-423n4 / 2022-02-skale-findings