Malicious user can steal innocent user's stake token reward by flashloan NFT

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L389-L396

Vulnerability details

Impact

Some NFT stake protocol like bored ape stake protocol support NFT holder deposit and withdraw their token to stake pool with their NFT token to gain more stake yield:

function _depositNft(uint256 _poolId, SingleNft[] calldata _nfts) private {
    updatePool(_poolId);
    uint256 tokenId;
    uint256 amount;
    Position storage position;
    uint256 length = _nfts.length;
    uint256 totalDeposit;
    for(uint256 i; i < length;) {
        tokenId = _nfts[i].tokenId;
        position = nftPosition[_poolId][tokenId];
        if (position.stakedAmount == 0) {
            if (nftContracts[_poolId].ownerOf(tokenId) != msg.sender) revert CallerNotOwner();
        }
        amount = _nfts[i].amount;
        _depositNftGuard(_poolId, position, amount);
        totalDeposit += amount;
        emit DepositNft(msg.sender, _poolId, amount, tokenId);
        unchecked {
            ++i;
        }
    }
    if (totalDeposit > 0) apeCoin.transferFrom(msg.sender, address(this), totalDeposit);
}

function _withdrawNft(uint256 _poolId, SingleNft[] calldata _nfts, address _recipient) private {
    updatePool(_poolId);
    uint256 tokenId;
    uint256 amount;
    uint256 length = _nfts.length;
    uint256 totalWithdraw;
    Position storage position;
    for(uint256 i; i < length;) {
        tokenId = _nfts[i].tokenId;
        if (nftContracts[_poolId].ownerOf(tokenId) != msg.sender) revert CallerNotOwner();

        amount = _nfts[i].amount;
        position = nftPosition[_poolId][tokenId];
        if (amount == position.stakedAmount) {
            uint256 rewardsToBeClaimed = _claim(_poolId, position, _recipient);
            emit ClaimRewardsNft(msg.sender, _poolId, rewardsToBeClaimed, tokenId);
        }
        _withdraw(_poolId, position, amount);
        totalWithdraw += amount;
        emit WithdrawNft(msg.sender, _poolId, amount, _recipient, tokenId);
        unchecked {
            ++i;
        }
    }
    if (totalWithdraw > 0) apeCoin.transfer(_recipient, totalWithdraw);
}

So a malicious user can flashloan such kind of NFT from this protocol and claim their stake token from the stake pool if the user stake their token by NFT before.

Proof of Concept

1. A bored ape NFT user who stake his ape token into bored ape stake protocol with his bored ape NFT before, and he use delegate protocol to delegate his NFT.
2. The malicious user monitor the innocent user's stake tx and scan the delegate tx, find the user staked his ape token into the APE stake contract before.
3. The malicious user flashloan the innocent user's bored ape NFT and claim the user's ape token from the APE stake pool to withdraw the user's staked ape token and rewards.
4. The malicious user return the bored NFT back to protocol to finish the tx.

Tools Used

vscode, manual review

Recommended Mitigation Steps

Warning the user should notice their stake token before they delegate their NFT to protocol, or consider blacklist mechanism to forbid such NFT delegate to the protocol.

Another easier option is close the flashloan function.

Assessed type

ERC721

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

0xfoobar (sponsor) disputed

[0xfoobar]

The explicit purpose of a Delegate Token is to offer utility access, if there's utility attached then the point of the flashloan is to be able to access it. Feature not a bug, users should not sell things they don't want to sell, and should not attach utility they do not wish others to access.

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[GalloDaSballo]

Agree with the Sponsor

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid