Architectural Pattern of Internal and External Functions Increases Attack Surface
There is an architectural pattern throughout the code of functions being defined in two places: an external wrapper (name) that verifies authorization and validates parameters, and an internal function (_name) that contains the implementation logic. This pattern separates concerns and avoids redundancy in the case that more than one external function reuses the same internal logic.\\nFor example, `VotingTokenLockupPlans.setupVoting` calls an internal function `_setupVoting` and sets the `holder` parameter to `msg.sender`.\\n```\\nfunction setupVoting(uint256 planId) external nonReentrant returns (address votingVault) {\\n votingVault = \\_setupVoting(msg.sender, planId);\\n```\\n\\n```\\nfunction \\_setupVoting(address holder, uint256 planId) internal returns (address) {\\n require(ownerOf(planId) == holder, '!owner');\\n```\\n\\nIn this case, however, there is no case in which `holder` should not be set to `msg.sender`. Because the internal function doesn't enforce this, it's theoretically possible that if another internal (or derived) function were compromised then it could call `_setupVoting` with `holder` set to `ownerOf(planId)`, even if `msg.sender` isn't the owner. This increases the attack surface through providing unneeded flexibility.\\nOther Examples\\n```\\nfunction segmentPlan(\\n uint256 planId,\\n uint256[] memory segmentAmounts\\n) external nonReentrant returns (uint256[] memory newPlanIds) {\\n newPlanIds = new uint256[](segmentAmounts.length);\\n for (uint256 i; i < segmentAmounts.length; i++) {\\n uint256 newPlanId = \\_segmentPlan(msg.sender, planId, segmentAmounts[i]);\\n```\\n\\n```\\nfunction \\_segmentPlan(address holder, uint256 planId, uint256 segmentAmount) internal returns (uint256 newPlanId) {\\n require(ownerOf(planId) == holder, '!owner');\\n```\\n\\n```\\nfunction revokePlans(uint256[] memory planIds) external nonReentrant {\\n for (uint256 i; i < planIds.length; i++) {\\n \\_revokePlan(msg.sender, planIds[i]);\\n```\\n\\n```\\nfunction \\_revokePlan(address vestingAdmin, uint256 planId) internal {\\n Plan memory plan = plans[planId];\\n require(vestingAdmin == plan.vestingAdmin, '!vestingAdmin');\\n```\\n
Resolution\\nFixed as of commit `f4299cdba5e863c9ca2d69a3a7dd554ac34af292`.\\nTo reduce the attack surface, consider hard coding parameters such as `holder` to `msg.sender` in internal functions when extra flexibility isn't needed.
```\\nfunction setupVoting(uint256 planId) external nonReentrant returns (address votingVault) {\\n votingVault = \\_setupVoting(msg.sender, planId);\\n```\\n