Omniscia AmpleSense Audit
StakingERC721 Manual Review Findings
StakingERC721 Manual Review Findings
SEC-01M: Inexistent Validation of Unstake Length
Type | Severity | Location |
---|---|---|
Input Sanitization | Minor | StakingERC721.sol:L81, L82, L86, L88 |
Description:
The unstake
function does not properly validate that the amount
that is being unstaked is at most equal to the amount of NFTs in the corresponding tokenOwnership
dynamic array, meaning that the function may fail to execute if an amount
greater than tokens.length
is specified.
Example:
76/**77 @dev Unstakes a certain amount of tokens, this SHOULD return the given amount of tokens to the account, if unstaking is currently not possible the function MUST revert78 @param amount Amount of tokens to remove from the stake79 @param isTokenA true if unstaking tokenA80*/81function unstake(uint256 amount, bool isTokenA) external {82 stakingContractEth.unstakeFrom(msg.sender, amount);83
84 uint256[] storage tokens;85 if(isTokenA)86 tokens = tokenOwnershipA[msg.sender];87 else88 tokens = tokenOwnershipB[msg.sender];89
90 for(uint i = 0; i < amount; i++) {91 uint256 id = tokens[tokens.length - 1];92 tokens.pop();93 if(isTokenA)94 tokenA.transferFrom(address(this), msg.sender, id);95 else96 tokenB.transferFrom(address(this), msg.sender, id);97 }98
99 emit Unstaked(msg.sender, amount, totalStakedFor(msg.sender));100 emit StakeChanged(stakingContractEth.totalStaked(), block.timestamp);101}
Recommendation:
We strongly recommend some form of validation to be imposed here. Additionally, we advise the AmpleSense team to introduce a function that allows a particular ID from the NFT ID array to be extracted as NFTs are unique and retaining a FIFO queue for the NFTs staked is ill-advised as a user may want a particular NFT unlocked and would have to consequently remove all NFTs that precede it.
Alleviation:
Proper unstaking length validation has been introduced in the codebase in the form of a require
check.