Description: The divideNft function applies the onlyNftOwner modifier twice, resulting in redundant ownership checks for the same nftAddress and tokenId. This duplication unnecessarily increases the gas cost for calling the function without adding any additional security or functionality.
Impact: While this issue does not introduce any vulnerabilities or impact the correctness of the function, it leads to inefficiencies in gas usage, making the function more expensive to execute for users.
Proof of Concept:
First add the following code to TokenDivider.sol file.
function divideNftOnce(address nftAddress, uint256 tokenId, uint256 amount) onlyNftOwner(nftAddress, tokenId) external {
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
if(amount == 0) { revert TokenDivider__AmountCantBeZero(); }
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol())));
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
emit NftDivided(nftAddress, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if(!transferSuccess) {
revert TokenDivider__TransferFailed();
}
}
Next add the following code to the TokenDividerTest.t.sol file.
function test_GasUsageForModifiers() public {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
uint256 startGasTwice;
uint256 endGasTwice;
uint256 gasUsedTwice;
startGasTwice = gasleft();
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
endGasTwice = gasleft();
gasUsedTwice = startGasTwice - endGasTwice;
console.log("Gas used for divideNftDoubleModiefiers: ", gasUsedTwice);
vm.stopPrank();
}
function test_GasUsageForModifier() public{
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
uint256 startGasOnce;
uint256 endGasOnce;
uint256 gasUsedOnce;
startGasOnce = gasleft();
tokenDivider.divideNftOnce(address(erc721Mock), TOKEN_ID, AMOUNT);
endGasOnce = gasleft();
gasUsedOnce = startGasOnce - endGasOnce;
console.log("Gas used for divideNftOneModifier: ", gasUsedOnce);
vm.stopPrank();
}
Recommended Mitigation: The proper solution to this is to remove the second unnecessary modifier
