code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/NativeTokenFactory.sol#L92

Vulnerability details

Impact

The YieldBox vault is vulnerable to an DoS attack which has the potential to disable the native token creation.

The problem being that createToken function used for native token creation is using assets.length.to32() as unique identifier for the next token, and the new added token are at the same time pushed to this array. Once assets.length become greater than uint32.max (4,294,967,295), calling createToken will always revert, hence this DoS situation. Furthermore, registerAsset can also be directly called (which add to the same assets array) to create ERC20, ERC721 or ERC1155 token.

Since token creation is permissionless, and the input data are easy to fake, it's possible to achieve this, of course it will cost some money to creates all those 4 Billions tokens XD, but on L2 or very cheap network, that is definatelly an achievable grief against the protocol depending of what is at stake for the attacker.

Putting this as Medium only due to the fact that the probability to be exploited is still low from my perspective.

Proof of Concept

The following test added to NativeTokenFactory.ts show how to create 100 identical tokens (only different for the tokenId) which are all the same, and the final assertion at the end confirm the array length being above 100.

    it.only("can create a 100 tokens", async function () {
        const count = 100;
        for (let i = 0; i < count; i++) {
            await expect(factory.createToken("Test Token", "TEST", 12, ""))
                .to.emit(factory, "URI")
                .withArgs("", i + 2)
                .to.emit(factory, "AssetRegistered")
                .withArgs(TokenType.Native, Zero, Zero, i + 2, i + 2)
                .to.emit(factory, "TokenCreated")
                .withArgs(Deployer, "Test Token", "TEST", 12, i + 2)

            expect(await factory.owner(i + 2)).equals(Deployer)

            const asset = await factory.assets(i + 2)
            expect(asset.tokenType).equals(TokenType.Native)
            expect(asset.contractAddress).equals(Zero)
            expect(asset.strategy).equals(Zero)
            expect(asset.tokenId).equals(i + 2)

            const token = await factory.nativeTokens(i + 2)
            expect(token.name).equals("Test Token")
            expect(token.symbol).equals("TEST")
            expect(token.decimals).equals(12)
        }

        expect(await factory.assetCount()).equals(count + 2)
    })

Tools Used

VS code, test suite

Recommended Mitigation Steps

Since nativeTokens is supporting uint256 already as well as all others functions using tokenId, I don't understand why you would want to downcast it to uint32, I would simply remove the to32(), and you would be bound to uint256 which is much stronger bound.

Assessed type

DoS

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

Consider QA

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)