Yield Staking Zap #31
Conversation
|
This pull request has been linked to Shortcut Story #529: Yield Staking Zap. |
# Conflicts: # contracts/solidity/NFTXInventoryStaking.sol
|
Hey @tomwade. I'm concerned about removing Originally we had only the The timelockMintFor function on NFTXLPStaking.sol works differently because it makes sure the necessary SLP tokens are being deposited, so there is no way for any 3rd parties (who have been added to the fee exclusion list) to infinite mint xSLP tokens. I think the benefit of the inventory staking version of timelockMintFor is that it is less gas. NFTXStakingZap handles passing in NFTs/vTokens and then just tells the NFTXInventoryStaking how many xTokens to mint. Whereas with LP staking, the NFTXStakingZap has to pass the SLP tokens to NFTXLPStaking. I'm guessing that the best option would be for us to change NFTXInventoryStaking so that it mimics NFTXLPStaking. Then it would be possible for us to remove the require(zapContract()) check. Next step would probably be checking with Kiwi about this. |
| @@ -110,8 +110,7 @@ contract NFTXInventoryStaking is PausableUpgradeable, UpgradeableBeacon, INFTXIn | |||
|
|
|||
| function timelockMintFor(uint256 vaultId, uint256 amount, address to, uint256 timelockLength) external virtual override returns (uint256) { | |||
| onlyOwnerIfPaused(10); | |||
| require(msg.sender == nftxVaultFactory.zapContract(), "Not staking zap"); | |||
There was a problem hiding this comment.
Removing this could allow any fee excluded address to unlimited mint any about of tokens
There was a problem hiding this comment.
Good point, I've added this back but altered the logic so that it looks in a mapping, rather than a single address match
|
|
||
| /// @notice A mapping of NFTX Vault IDs to their address corresponding | ||
| /// vault contract address | ||
| mapping(uint256 => address) public nftxVaultAddresses; |
There was a problem hiding this comment.
Why is this being saved, the factory stores this already no?
There was a problem hiding this comment.
This just creates gas savings by allowing us by bypassing subsequent calls to the factory contract and instead just looking up the internal ID -> address mapping. Isn't an astronomical saving, but does add up.
| // Get our start WETH balance | ||
| uint wethBalance = WETH.balanceOf(address(this)); | ||
|
|
||
| // Wrap ETH into WETH for our contract from the sender | ||
| if (msg.value > 0) { | ||
| WETH.deposit{value: msg.value}(); | ||
| } | ||
|
|
||
| // Get our vaults base staking token. This is used to calculate the xToken | ||
| address baseToken = _vaultAddress(vaultId); | ||
| require(baseToken != address(0), 'Invalid vault provided'); |
There was a problem hiding this comment.
| // Get our start WETH balance | |
| uint wethBalance = WETH.balanceOf(address(this)); | |
| // Wrap ETH into WETH for our contract from the sender | |
| if (msg.value > 0) { | |
| WETH.deposit{value: msg.value}(); | |
| } | |
| // Get our vaults base staking token. This is used to calculate the xToken | |
| address baseToken = _vaultAddress(vaultId); | |
| require(baseToken != address(0), 'Invalid vault provided'); | |
| // Get our vaults base staking token. This is used to calculate the xToken | |
| address baseToken = _vaultAddress(vaultId); | |
| require(baseToken != address(0), 'Invalid vault provided'); | |
| // Get our start WETH balance | |
| uint wethBalance = WETH.balanceOf(address(this)); | |
| // Wrap ETH into WETH for our contract from the sender | |
| if (msg.value > 0) { | |
| WETH.deposit{value: msg.value}(); | |
| } |
Reorder for readability
| // Make a direct timelock mint using the default timelock duration. This sends directly | ||
| // to our user, rather than via the zap, to avoid the timelock locking the tx. | ||
| IERC20Upgradeable(baseToken).transfer(inventoryStaking.vaultXToken(vaultId), vaultTokenAmount); | ||
| inventoryStaking.timelockMintFor(vaultId, vaultTokenAmount, msg.sender, 2); |
There was a problem hiding this comment.
No timelock is being applied here, is that ok?
| */ | ||
|
|
||
| receive() external payable { | ||
| // |
There was a problem hiding this comment.
This should probably revert so if anyone just randomly sends ETH it doesn't get stuck
| */ | ||
|
|
||
| function setSwapTarget(address payable _swapTarget) external onlyOwner { | ||
| swapTarget = _swapTarget; |
There was a problem hiding this comment.
Could malicious ownership allow this zap to perform the vulnerability?
|
|
||
| function _vaultAddress(uint256 vaultId) internal returns (address) { | ||
| if (nftxVaultAddresses[vaultId] == address(0)) { | ||
| nftxVaultAddresses[vaultId] = nftxFactory.vault(vaultId); |
There was a problem hiding this comment.
Not sure why this is needed when you can just use the factory no?
There was a problem hiding this comment.
Mentioned briefly in previous comment, but it is for gas savings for repeat calls
There was a problem hiding this comment.
Do you have a rough estimate on savings?
Story details: https://app.shortcut.com/nftx/story/529