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