29213 - [SC - High] The function always revert if _stakeNFT True d...
Submitted on Mar 10th 2024 at 20:34:07 UTC by @stiglitz for Boost | ZeroLend
Report ID: #29213
Report type: Smart Contract
Report severity: High
Target: https://github.com/zerolend/governance
Impacts:
Permanent freezing of funds
Temporary freezing of funds for at least 1 hour
Description
Brief/Intro
See the function safeTransferFrom::_createLock:
if (_stakeNFT) {_mint(address(this), _tokenId);bytesmemory data = abi.encode(_to);safeTransferFrom(address(this),address(staking), _tokenId, data);} else_mint(_to, _tokenId);
if _stakeNFT == True, the tx always reverts because msg.sender is the user who called createLock, and the user does not have approval for moving NFT owned by someone else. As we can see in the code, the NFT is minted to address(this), so the user is not the owner, and he needs approval from the contact.
However, the problem imho is the fact that the NFT is not even owned by a user but by the contract itself. Because if approval is implemented to make the transfer succeed, the user has no ability to get what is his.
Vulnerability Details
Let's also look at the OmnichainStaking::onERC721Received:
if (msg.sender ==address(lpLocker)) { lpPower[tokenId] = lpLocker.balanceOfNFT(tokenId);_mint(from, lpPower[tokenId] *4);}
And also the function OmnichainStaking::unstakeLP:
Lets say we have the approval and we create a lock with _stakeNFT == True. The function safeTransferFrom is executed where from == address(this) which is BaseLocker (LockerLP). NOT A USER.
Who is gonna call unstakeLP ? The tokens are owner by the LockerLP contract, and only the contract can unstake them.
Impact Details
The protocol does not work as it is supposed to, and if the function with _stakeNFT == True is called and it works (it does not right now) - The user will basically lose his invested money into veNFT and the voting power will be useless owned by the protocol contract.
I think the impact is very high and I would say critical. But because of the missing approval, the function does not work at all, so the impact is not there :D
References
Add any relevant links to documentation or code
Proof of Concept
Ve mock token
pragmasolidity ^0.8.6;import"@openzeppelin/contracts/token/ERC20/ERC20.sol";// Example class - a mock class derived from ERC20contractVeTokenisERC20 {constructor(uint256 initialBalance) ERC20("Ve Token", "VT") public {_mint(msg.sender, initialBalance); }}
Re mock token
pragmasolidity ^0.8.6;import"@openzeppelin/contracts/token/ERC20/ERC20.sol";// Example class - a mock class derived from ERC20contractReTokenisERC20 {constructor(uint256 initialBalance) ERC20("Re Token", "RT") public {_mint(msg.sender, initialBalance); }}
X contract (ERC721 compatible)
import {ILocker} from"../contracts/interfaces/ILocker.sol";import {OmnichainStaking} from"../contracts/locker/OmnichainStaking.sol";import {PoolVoter} from"../contracts/voter/PoolVoter.sol";contract X { ILocker public lpLocker; OmnichainStaking public staking; PoolVoter public poolVoter;constructor(address_lpLocker,address_staking,address_poolVoter){ lpLocker =ILocker(_lpLocker); staking =OmnichainStaking(_staking); poolVoter =PoolVoter(_poolVoter); }functiononERC721Received(address,address from,uint256 tokenId,bytescalldata data ) externalreturns (bytes4) {//lpLocker.safeTransferFrom(address(this), lpLocker, tokenId);returnthis.onERC721Received.selector; }// Unstake from omnichain stakingfunctionunstakeLP(uint256 tokenId) external { staking.unstakeLP(tokenId); }// This allows me to send anywhere I wantfunctionsend(address to,uint256 tokenId) external { lpLocker.safeTransferFrom(address(this), to, tokenId); }functionvote(address[] calldata_poolVote,uint256[] calldata_weights) external { poolVoter.vote(_poolVote, _weights); }}