sherlock-audit / 2023-12-avail-judging

4 stars 4 forks source link

m4ttm - Lack of zero checks in updateTokens allows AvailBridge assetId to be set #134

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

m4ttm

medium

Lack of zero checks in updateTokens allows AvailBridge assetId to be set

Summary

AvailBridge uses assetIds to map bytes32 values to ERC20 tokens. These are provided as arguments by users when referring to ERC20 assets. In a message the native token Avail has the zero ID but setting its asset ID to its contract would allow it to be used with the generic sendERC20 method.

Vulnerability Detail

A separate method is provided to bridge Avail but if its address is added to the tokens mapping, it will be possible to use the generic method for ERC20s which won't mint and burn Avail as intended. The admin is able to do so due to the lack of zero checks.

Impact

This could allow users to use the contract in a way that wasn't intended and tokens will not be minted and burned as the bridge is used.

Code Snippet

https://github.com/sherlock-audit/2023-12-avail/blob/1afb56b8d4dfbf5d3f21bed0ddf80a04730204b5/contracts/src/AvailBridge.sol#L132-L147

function updateTokens(bytes32[] calldata assetIds, address[] calldata tokenAddresses)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        uint256 length = assetIds.length;
        if (length != tokenAddresses.length) {
            revert ArrayLengthMismatch();
        }
        for (uint256 i = 0; i < length;) {
            tokens[assetIds[i]] = tokenAddresses[i];
            unchecked {
                ++i;
            }
        }
    }

Tool used

Manual Review

Recommendation

Add zero checks to updateTokens.

sherlock-admin commented 8 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Invalid. Admin mistake

takarez commented:

invalid because { invalid: admin function}