Users can add 6 decimal token funds for free

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L1068-L1109

Vulnerability details

There is a logic error in _convertDecimals() function which means wrapping interactions for tokens with less than 18 decimals are processed incorrectly.

The below is triggered in _convertDecimals() where the input parameter decimals is less than 18. If amountToConvert is less than shift and then convertedAmount returns 0

        } else {
            // Decimal shift right (remove precision) -> truncation
            uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
            convertedAmount = amountToConvert / shift;
            truncatedAmount = amountToConvert % shift; // catch the dust
        }

In _determineTransferAmount if truncatedAmount is > 0 a 1 will be added to transferAmount.

        (transferAmount, truncated) = _convertDecimals(NORMALIZED_DECIMALS, decimals, amount);

It ends up that only "1" will be transferred from the user to ocean. The user will keep most of their money minus "1" in the ERC20 token balances and they will also get the an amount of Ocean 1155 tokens equivalent to the amoiunt they tried to wrap.

Impact

Users can get free money when wrapping a 6 decimal token

Proof of Concept

Add this test to TokenIntegration.js and run:

    describe("ERC-20, 6-decimals", () => {
      const decimals = "6";
      const mintAmount = 100000000;

      let token;
      let oceanId;
      let owner;

      before("Deploy token", async () => {
        const erc20Contract = await ethers.getContractFactory("ERC20MintsToDeployer");
        token = await erc20Contract.deploy(mintAmount, decimals);
        oceanId = shellV2.utils.calculateWrappedTokenId({ address: token.address, id: 0 });
        owner = await ocean.owner();

        const [aliceBalance, oceanBalance] = await Promise.all([
          token.balanceOf(alice.address),
          token.balanceOf(ocean.address),
        ]);

        expect(aliceBalance).to.equal(mintAmount);
        expect(oceanBalance).to.equal(0);
      });

      it.only("Wrap & Unwrap 6 Decimal ERC-20", async () => {
        const wrapInteraction = shellV2.interactions.wrapERC20({
          address: token.address,
          amount: mintAmount,
        });

        await token.connect(alice).approve(ocean.address, mintAmount);
        await shellV2.executeInteraction({ ocean: ocean, signer: alice, interaction: wrapInteraction });

        console.log("await token.balanceOf(alice.address)", await token.balanceOf(alice.address));

        expect(await ocean.balanceOf(alice.address, oceanId)).to.equal(mintAmount);
        expect(await token.balanceOf(ocean.address)).to.equal(1);
        expect(await token.balanceOf(alice.address)).to.equal(mintAmount - 1);
      });
    });

Tools Used

Hardhat Manual Review

Recommended Mitigation Steps

If user has truncatedAmount but it is less than shift do not add 1 to transferAmount

function _determineTransferAmount(
    uint256 amount,
    uint8 decimals
)
    private
    pure
    returns (uint256 transferAmount, uint256 dust)
{
    (transferAmount, ) = _convertDecimals(NORMALIZED_DECIMALS, decimals, amount);

    (uint256 normalizedTransferAmount, ) =
        _convertDecimals(decimals, NORMALIZED_DECIMALS, transferAmount);

    if (normalizedTransferAmount > amount) {
        dust = normalizedTransferAmount - amount;
    } else {
        dust = 0;
    }
}

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #14

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid