Reentrancy in Vesting.sol:claim() will allow users to drain the contract due to executing .call() on user's address before setting s.index = uint128(i)
Reentrancy in Vesting.sol:claim() will allow users to drain the contract due to executing .call() on user's address before setting s.index = uint128(I)\\nHere is the Vesting.sol:claim() function:\\n```\\nfunction claim() external {\\n        address sender = msg.sender;\\n\\n        UserDetails storage s = userdetails[sender];\\n        require(s.userDeposit != 0, "No Deposit");\\n      require(s.index != vestingPoints.length, "already claimed");\\n        uint256 pctAmount;\\n        uint256 i = s.index;\\n        for (i; i <= vestingPoints.length - 1; i++) {\\n            if (block.timestamp >= vestingPoints[i][0]) {\\n                pctAmount += (s.userDeposit * vestingPoints[i][1]) / 10000;\\n            } else {\\n                break;\\n            }\\n        }\\n        if (pctAmount != 0) {\\n            if (address(token) == address(1)) {\\n              (bool sent, ) = payable(sender).call{value: pctAmount}("");\\n                require(sent, "Failed to send BNB to receiver");\\n            } else {\\n                token.safeTransfer(sender, pctAmount);\\n            }\\n          s.index = uint128(i);\\n            s.amountClaimed += pctAmount;\\n        }\\n    }\\n```\\n\\nFrom the above, You'll notice the claim() function checks if the caller already claimed by checking if the s.index has already been set to vestingPoints.length. You'll also notice the claim() function executes .call() and transfer the amount to the caller before setting the s.index = uint128(i), thereby allowing reentrancy.\\nLet's consider this sample scenario:\\nAn attacker contract(alice) has some native pctAmount to claim and calls `claim()`.\\n"already claimed" check will pass since it's the first time she's calling `claim()` so her s.index hasn't been set\\nHowever before updating Alice s.index, the Vesting contract performs external .call() to Alice with the amount sent as well\\nAlice reenters `claim()` again on receive of the amount\\nbypass index "already claimed" check since this hasn't been updated yet\\ncontract performs external .call() to Alice with the amount sent as well again,\\nSame thing happens again\\nAlice ends up draining the Vesting contract
Here is the recommended fix:\\n```\\nif (pctAmount != 0) {\\n// Add the line below\\n           s.index = uint128(i);\\n            if (address(token) == address(1)) {\\n                (bool sent, ) = payable(sender).call{value: pctAmount}("");\\n                require(sent, "Failed to send BNB to receiver");\\n            } else {\\n                token.safeTransfer(sender, pctAmount);\\n            }\\n// Remove the line below\\n           s.index = uint128(i);\\n            s.amountClaimed // Add the line below\\n= pctAmount;\\n        }\\n```\\n\\nI'll also recommend using reentrancyGuard.
Reentrancy in Vesting.sol:claim() will allow users to drain the contract
```\\nfunction claim() external {\\n        address sender = msg.sender;\\n\\n        UserDetails storage s = userdetails[sender];\\n        require(s.userDeposit != 0, "No Deposit");\\n      require(s.index != vestingPoints.length, "already claimed");\\n        uint256 pctAmount;\\n        uint256 i = s.index;\\n        for (i; i <= vestingPoints.length - 1; i++) {\\n            if (block.timestamp >= vestingPoints[i][0]) {\\n                pctAmount += (s.userDeposit * vestingPoints[i][1]) / 10000;\\n            } else {\\n                break;\\n            }\\n        }\\n        if (pctAmount != 0) {\\n            if (address(token) == address(1)) {\\n              (bool sent, ) = payable(sender).call{value: pctAmount}("");\\n                require(sent, "Failed to send BNB to receiver");\\n            } else {\\n                token.safeTransfer(sender, pctAmount);\\n            }\\n          s.index = uint128(i);\\n            s.amountClaimed += pctAmount;\\n        }\\n    }\\n```\\n