[M-1] ERC20 Tokens sent over the bridge may get locked, if the allowed token-list updates, before the ERC20 are received.

Summary

The AvailBridge may lock funds, if the allowed token list is updated, while there are still tokens in the contract, which were not yet received by AvailBridge::receiveERC20

Vulnerability Detail

AvailBridge::sendERC20 and AvailBridge::receiveERC20 both use the following check:

address token = tokens[assetId];
    if (token == address(0)) {
        revert InvalidAssetId();
}

If an ERC20 is sent via AvailBridge::sendERC20 and the token is specified in the tokens-mapping, the transaction will go through and the corresponding amount of the ERC20 is stored inside the AvailBridge contract. To receive the ERC20, a user must call the AvailBridge:receiveERC20 function. If the tokens mapping is updated, before a user can withdraw their tokens (which can be done anytime through a trusted admin), the corresponding assetId will point to address(0). The function AvailBridge::receiveERC20 will then revert with error InvalidAssetId, locking the tokens inside the contract permanently or until the token is added to the mapping again.

Impact

Impact: High Likelihood: Low => Medium

Code Snippet

Add the following test to the bottom of the AvailBridgeTest.t.sol file and run the test with forge test --mt test_auditSendERC20 -vvvvv

function test_auditSendERC20(bytes32 assetId, bytes32 to, uint256 amount) external {
        // default test_sendERC20 test to update tokens and send
        vm.assume(to != bytes32(0) && amount != 0);
        address from = makeAddr("from");
        ERC20Mock token = new ERC20Mock();
        token.mint(from, amount);
        bytes32[] memory assetIdArr = new bytes32[](1);
        assetIdArr[0] = assetId;
        address[] memory tokenArr = new address[](1);
        tokenArr[0] = address(token);
        vm.prank(owner);
        bridge.updateTokens(assetIdArr, tokenArr);
        IAvailBridge.Message memory message =
            IAvailBridge.Message(0x02, bytes32(bytes20(from)), to, 2, 1, abi.encode(assetId, amount), 0);
        vm.prank(from);
        token.approve(address(bridge), amount);
        vm.expectCall(address(token), abi.encodeCall(token.transferFrom, (from, address(bridge), amount)));
        vm.prank(from);
        bridge.sendERC20(assetId, to, amount);
        assertEq(bridge.isSent(0), keccak256(abi.encode(message)));
        assertEq(token.balanceOf(from), 0);
        assertEq(token.balanceOf(address(bridge)), amount);

        // update tokens to empty lists
        bytes32[] memory newAssetIdArr = new bytes32[](1);
        address[] memory newTokenArr = new address[](1);
        vm.prank(owner);
        bridge.updateTokens(newAssetIdArr, newTokenArr);

        // try to receive the ERC20
        IAvailBridge.Message memory message2 =
            IAvailBridge.Message(0x02, bytes32(bytes20(from)), to, 1, 2, abi.encode(assetId, amount), 0);
        IAvailBridge.MerkleProofInput memory input = IAvailBridge.MerkleProofInput(
            new bytes32[](0), new bytes32[](0), bytes32(0), 0, bytes32(0), bytes32(0), bytes32(0), 0
        );
        bridge.receiveERC20(message2, input);
        assertFalse(bridge.isBridged(keccak256(abi.encode(message2))));
}

note: this test will fail with the InvalidAssetId error, because the tokens mapping is updated to an empty list, before the token was received. If fixed, this test would still fail due to the incorrect MerkleProofInput, which does not matter in this case, because the revert statement triggers first.

Tool used

foundry forge

Manual Review

Recommendation

Remove the check, if the token is whitelisted. An additional check, if there are enough tokens in the contracts balance and on the originally sent sendERC20 should be implemented.

Duplicate of #82

sherlock-audit / 2023-12-avail-judging

0xWallSecurity - [M-1] ERC20 Tokens sent over the bridge may get locked, if the allowed token-list updates, before the ERC20 are received. #100