Summary

The divideNft function in the TokenDivider.sol contract contains a redundant invocation of the onlyNftOwner modifier. This redundancy results in unnecessary computation, increasing gas consumption and potentially leading to confusion about the intended behavior of the function.

Vulnerability Details

The function definition is as follows:

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L109

Here, the onlyNftOwner modifier is applied twice with identical arguments. Since modifiers are executed sequentially before the function body, this leads to:

1. Redundant Validation: The same ownership check is performed twice for the same nftAddress and tokenId. This is unnecessary as a single invocation of the modifier suffices.

2. Increased Gas Costs: Each modifier invocation incurs gas costs. By duplicating the modifier, the gas consumption for this function is unnecessarily increased.

Impact

• Gas Inefficiency: Users interacting with this function will incur higher gas fees due to redundant operations.

Proof of Concept

Copy the code below and paste it in the TokenDividerTest.t.sol file:

Now run this in your terminal:

You should get this as your result:

Now go to the TokenDivider.sol contract and fix the error buy removing the second redundant modifier then run this test again. You should now get this as your result.

Now you can see the gas used in the second test after the error has been fixed is less than what was used in the first. Even though the difference may be little, redundant code in smart contracts should not be tolerated. This was just to show the impact on gas, but that is beside the main point.

Tools Used

• Manual Code Review

• Foundry

Recommendations

To address this issue, the function should be updated to include the onlyNftOwner modifier only once. This eliminates redundancy, reduces gas usage, and ensures the code remains maintainable and clear. The corrected function would look like this: