XDEFI contest - cmichel's results

Handle

cmichel

Vulnerability details

The XDEFIDistribution.updateDistribution function does not have re-entrancy locks and can be called from lock -> _lock -> safeMint which gives control back to the msg.sender if it is a contract by performing msg.sender.onERC721Received(...).

As the totalDepositedXDEFI is not yet updated with the locked amount at the time of calling safeMint, re-entering into updateDistribution will count the deposited amount as rewards and distribute them to all previous holders by increasing _pointsPerUnit.

POC

For simplicity, assume there's a zero-second locking duration and all these actions can be done in a single transaction. It also leads to loss of funds in the case where there's no zero-second locking duration.

• Attacker locks tokens, gets NFT_1
• Attacker locks tokens again from a smart contract calling lock and has an onERC721Received receiver function. The _lock's safeMint will call this function with NFT_2 and the attacker calls updateDistribution which distributes the locked amount as a reward to all previous stakers, including themself as part of owning NFT_1.
• Attacker unlocks NFT_1, receives the original lock amount in addition to part of the second lock's deposit amount.
• Attacker unlocks NFT_2, receives the original lock amount.
• The attacker's profit is part of the second lock amount. The share can be maximized by locking more tokens in NFT_1. The attacker's profit is the protocol's loss.

Impact

Funds can be stolen from the contract.

Recommended Mitigation Steps

1. Use _mint instead of _safeMint.
2. Add re-entrancy locks to updateDistribution and migrate.

Handle

cmichel

Vulnerability details

The XDEFIDistribution.merge function burns tokens, which decreases the ERC721Enumerable.totalSupply() and the _generateNewTokenId function returns a token ID as the concatenation of the points and totalSupply() + 1:

function _generateNewTokenId(uint256 points_) internal view returns (uint256 tokenId_) {
    // Points is capped at 128 bits (max supply of XDEFI for 10 years locked), total supply of NFTs is capped at 128 bits.
    return (points_ << uint256(128)) + uint128(totalSupply() + 1);
}

If a user is unlucky it can happen that there is a collision and they cannot merge their tokens as the _safeMint fails when trying to mint an already existing token ID.

POC

• user A has two token IDs with points 100 and 200
• Imagine the current totalSupply() = N and the second last token that was minted has 300 points. Its token id is: tokenId = (N - 1) << 128 | 300
• user A now tries to merge their tokens. The two burns decrease the total supply to N - 2. The mint uses totalSupply() + 1 = N - 2 + 1 = N - 1. The merged token ID is now also tokenId = (N-1) << 128 | 300. The merge fails as in ERC721._mint as this token already exists

Impact

Token merges can fail

Recommended Mitigation Steps

You probably don't want totalSupply() to ever decrease. Either don't call _burn and use a different way of invalidating these tokens. Or just use a counter for the lower bits of tokenId instead of totalSupply() + 1.

Handle

cmichel

Vulnerability details

The XDEFIDistribution._lock_ function mints a new token and the _generateNewTokenId function returns a token ID as the concatenation of the points and totalSupply() + 1:

function _generateNewTokenId(uint256 points_) internal view returns (uint256 tokenId_) {
    // Points is capped at 128 bits (max supply of XDEFI for 10 years locked), total supply of NFTs is capped at 128 bits.
    return (points_ << uint256(128)) + uint128(totalSupply() + 1);
}

Upon observing a lock call, an attacker can frontrun the transaction and forge a lock themselves that will use the same tokenId as the new call.

POC

Preattack step: Attacker prepares many tokens that can be used to reduce the totalSupply by burning them in merge. The attacker can use the shortest lock duration and an amount of 1. This step can be prepared at any time and it can be assumed that these tokens are already unlocked at the time of attack.

Attack step

• Imagine the current totalSupply() = N.
• The victim sends a lock transaction that would use P points.
• The Attacker frontruns this lock transaction with their own transaction that does the following.:
  • Calls lock to create a new token that also locks P points (at the lowest duration). The token will get tokenId = (N + 1) << 128 | P. The new totalSupply() = N + 1
  • The attacker merges two of the unlocked tokens they prepared in the preparation step. This changes the total supply to - 2 + 1 = -1. The new totalSupply() = (N + 1) - 1 = N.
• Victim's transaction is mined and the proposed token id is the same as the attacker's tokenId = (N + 1) << 128 | P. The transaction fails in _safeMint -> mint as the token already exists

Impact

All locks can be denied which is the main purpose of the contract. An attacker can prevent anyone else from staking tokens and therefore keep all rewards to themselves. While the attacker needs to potentially transfer a large amount of xDEFI to reach the desired points of the victim transaction, if there is a very low lock duration, they can just keep unlocking their tokens again and the capital requirement is low.

Recommended Mitigation Steps

Keep the lowest lock duration high (in the magnitude of days) to require a large capital requirement to consistently perform this attack. Alternatively, just use a tokenId that is collision-resistant, for example, a simple counter instead of using totalSupply() which can decrease.