0x52 - Anyone can withdraw user's Velo Deposit NFT after approval is given to depositor

[github-actions[bot]]

0x52

high

Anyone can withdraw user's Velo Deposit NFT after approval is given to depositor

Summary

Depositor#withdrawFromGauge is a public function that can be called by anyone which transfers token to msg.sender. withdrawFromGauge burns the NFT to be withdrawn, which means that Depositor must either be approved or be in possession of the NFT. Since it doesn't transfer the NFT to the contract before burning the user must either send the NFT to the Depositor or approve the Depositor in a separate transaction. After the NFT is either transferred or approved, a malicious user could withdraw the NFT for themselves.

Vulnerability Detail

function withdrawFromGauge(uint256 _NFTId, address[] memory _tokens)  public  {
    uint256 amount = depositReceipt.pooledTokens(_NFTId);
    depositReceipt.burn(_NFTId);
    gauge.getReward(address(this), _tokens);
    gauge.withdraw(amount);
    //AMMToken adheres to ERC20 spec meaning it reverts on failure, no need to check return
    //slither-disable-next-line unchecked-transfer
    AMMToken.transfer(msg.sender, amount);
}

Depositor#withdrawFromGauge allows anyone to call it, burning the NFT and sending msg.sender the withdrawn tokens.

function burn(uint256 _NFTId) external onlyMinter{
    require(_isApprovedOrOwner(msg.sender, _NFTId), "ERC721: caller is not token owner or approved");
    delete pooledTokens[_NFTId];
    delete relatedDepositor[_NFTId];
    _burn(_NFTId);
}

Depositor calls DepositReceipt_Base#burn, which means that it must be either the owner or approved for the NFT. Since Depositor#withdrawFromGauge doesn't transfer the NFT from the user, this must happen in a separate transaction. Between the user approval/transfer and them calling Depositor#withdrawFromGauge a malicious user could call Depositor#withdrawFromGauge first to withdraw the NFT and steal the users funds. This would be very easy to automate with a bot.

Example: User A deposits 100 underlying into their Depositor and is given Token A which represents their deposit. After some time they want to redeem Token A so they Approve their Depositor for Token A. User B sees the approval and quickly calls Depositor#withdrawFromGauge to withdraw Token A. User B is sent the 100 tokens and Token A is burned from User A.

Impact

Users attempting to withdraw can have their funds stolen

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/Depositor.sol#L119-L127

Tool used

Manual Review

Recommendation

Only allow owner of NFT to withdraw it:

    function withdrawFromGauge(uint256 _NFTId, address[] memory _tokens)  public  {
+       require(depositReceipt.ownerOf(_NFTId) == msg.sender);
        uint256 amount = depositReceipt.pooledTokens(_NFTId);
        depositReceipt.burn(_NFTId);
        gauge.getReward(address(this), _tokens);
        gauge.withdraw(amount);
        //AMMToken adheres to ERC20 spec meaning it reverts on failure, no need to check return
        //slither-disable-next-line unchecked-transfer
        AMMToken.transfer(msg.sender, amount);
    }

[kree-dotcom]

Fixed https://github.com/kree-dotcom/Velo-Deposit-Tokens/commit/23ff5653b555b11c9f4dead7ff5a72d50eac5788

Here we have added a check on line 81 and 122 as suggested. There is also minor refactoring which is needed due to the fact if we are doing a partial withdrawal then after calling depositReceipt.split() the owner of the newly acquired depositReceipt is the Depositor not the original msg.sender. Therefore we moved the withdrawal logic to an internal function that both withdrawFromGauge() and partialWithdrawFromGauge() both access after ownership checks.

[IAm0x52]

Fixes look good. Splits withdrawFromGauge into to an internal and external function with the external function checking for NFT ownership.