Omniscia AmpleSense Audit

StakingERC721 Manual Review Findings

StakingERC721 Manual Review Findings

SEC-01M: Inexistent Validation of Unstake Length

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:

contracts/StakingERC721.sol
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 revert
78 @param amount Amount of tokens to remove from the stake
79 @param isTokenA true if unstaking tokenA
80*/
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 else
88 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 else
96 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.