The reentrancy vulnerability in _safeMint can allow an attacker to steal all rewards

[code423n4]

Handle

cccz

Vulnerability details

Impact

There is a reentrancy vulnerability in the _safeMint function

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory _data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, _data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }
    ...
    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;

The lock function changes the totalDepositedXDEFI variable after calling the _safeMint function

    function lock(uint256 amount_, uint256 duration_, address destination_) external noReenter returns (uint256 tokenId_) {
        // Lock the XDEFI in the contract.
        SafeERC20.safeTransferFrom(IERC20(XDEFI), msg.sender, address(this), amount_);

        // Handle the lock position creation and get the tokenId of the locked position.
        return _lock(amount_, duration_, destination_);
    }
    ...
        function _lock(uint256 amount_, uint256 duration_, address destination_) internal returns (uint256 tokenId_) {
        // Prevent locking 0 amount in order generate many score-less NFTs, even if it is inefficient, and such NFTs would be ignored.
        require(amount_ != uint256(0) && amount_ <= MAX_TOTAL_XDEFI_SUPPLY, "INVALID_AMOUNT");

        // Get bonus multiplier and check that it is not zero (which validates the duration).
        uint8 bonusMultiplier = bonusMultiplierOf[duration_];
        require(bonusMultiplier != uint8(0), "INVALID_DURATION");

        // Mint a locked staked position NFT to the destination.
        _safeMint(destination_, tokenId_ = _generateNewTokenId(_getPoints(amount_, duration_)));

        // Track deposits.
        totalDepositedXDEFI += amount_;

Since the updateDistribution function does not use the noReenter modifier, the attacker can re-enter the updateDistribution function in the _safeMint function. Since the value of totalDepositedXDEFI is not updated at this time, the _pointsPerUnit variable will become abnormally large.

     function updateDistribution() external {
        uint256 totalUnitsCached = totalUnits;

        require(totalUnitsCached> uint256(0), "NO_UNIT_SUPPLY");

        uint256 newXDEFI = _toUint256Safe(_updateXDEFIBalance());

        if (newXDEFI == uint256(0)) return;

        _pointsPerUnit += ((newXDEFI * _pointsMultiplier) / totalUnitsCached);

        emit DistributionUpdated(msg.sender, newXDEFI);
    }
    ...
    function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) {
        uint256 previousDistributableXDEFI = distributableXDEFI;
        uint256 currentDistributableXDEFI = distributableXDEFI = IERC20(XDEFI).balanceOf(address(this))-totalDepositedXDEFI;

        return _toInt256Safe(currentDistributableXDEFI)-_toInt256Safe(previousDistributableXDEFI);
    }

If the attacker calls the lock function to get the NFT before exploiting the reentrance vulnerability, then the unlock function can be called to steal a lot of rewards, and the assets deposited by the user using the reentrance vulnerability can also be redeemed by calling the unlock function. Since the unlock function calls the _updateXDEFIBalance function, the attacker cannot steal the assets deposited by the user

     function unlock(uint256 tokenId_, address destination_) external noReenter returns (uint256 amountUnlocked_) {
        // Handle the unlock and get the amount of XDEFI eligible to withdraw.
        amountUnlocked_ = _unlock(msg.sender, tokenId_);

        // Send the the unlocked XDEFI to the destination.
        SafeERC20.safeTransfer(IERC20(XDEFI), destination_, amountUnlocked_);

        // NOTE: This needs to be done after updating `totalDepositedXDEFI` (which happens in `_unlock`) and transferring out.
        _updateXDEFIBalance();
    }
...
     function _unlock(address account_, uint256 tokenId_) internal returns (uint256 amountUnlocked_) {
        // Check that the account is the position NFT owner.
        require(ownerOf(tokenId_) == account_, "NOT_OWNER");

        // Fetch position.
        Position storage position = positionOf[tokenId_];
        uint96 units = position.units;
        uint88 depositedXDEFI = position.depositedXDEFI;
        uint32 expiry = position.expiry;

        // Check that enough time has elapsed in order to unlock.
        require(expiry != uint32(0), "NO_LOCKED_POSITION");
        require(block.timestamp >= uint256(expiry), "CANNOT_UNLOCK");

        // Get the withdrawable amount of XDEFI for the position.
        amountUnlocked_ = _withdrawableGiven(units, depositedXDEFI, position.pointsCorrection);

        // Track deposits.
        totalDepositedXDEFI -= uint256(depositedXDEFI);

        // Burn FDT Position.
        totalUnits -= units;
        delete positionOf[tokenId_];

        emit LockPositionWithdrawn(tokenId_, account_, amountUnlocked_);
    }
    ...
     function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) {
        return
            (
                _toUint256Safe(
                    _toInt256Safe(_pointsPerUnit * uint256(units_)) +
                    pointsCorrection_
                ) / _pointsMultiplier
            ) + uint256(depositedXDEFI_);
    }

Proof of Concept

https://github.com/XDeFi-tech/xdefi-distribution/blob/v1.0.0-beta.0/contracts/XDEFIDistribution.sol#L253-L281

Tools Used

Manual analysis

Recommended Mitigation Steps

-    function updateDistribution() external  {
+    function updateDistribution() external  noReenter {

[deluca-mike]

Valid and a big issue. However, due to other recommendations, I will not solve it this way. Instead, updateDistribution() will be called at the start of every lock/unlock function (so it can't have a noReenter modifier), and the _safeMint calls will be moved to the end of their respective operations to prevent the effect of the re-entrancy (i.e. position will created with a _pointsPerUnit before a re-entering from _safeMint can affect it). Tests will be added to show this is not longer possible.

[deluca-mike]

In our release candidate contract, as mentioned above, updateDistribution() is called before each locking and unlocking function, via a updatePointsPerUnitAtStart modifier, and thus, updateDistribution() is now a public fucntion, and since it is used by other functions, cannot be behind a noReenter.

See:

• lock
• lockWithPermit
• relock
• unlock
• relockBatch
• unlockBatch

Also, a test was written to ensure that this is no longer exploitable, and that the contract behaves properly if a re-entrancy call updateDistribution().

[Ivshti]

Agreed with the severity.

Resolution of reordering the calls seems to be adequate