XDEFI contest - kenzo's results

Handle

kenzo

Vulnerability details

The lock function calls safeMint in the midst of it. This can lead to reentry to updateDistribution, thereby ruining contract invariants.

Impact

At worst case, loss of user funds: distributableXDEFI will be set to be bigger than it really is, and so when a legitimate user will try to unlock his tokens, the tx will fail because the contract will try sending the user rewards which the contract does not have. There might be other ways to exploit the reentrancy but I believe this simple example is enough to show that this is a high risk issue.

Proof of Concept

lock function safeMints to a user before totalDepositedXDEFI has been updated. (Code ref) The user can craft a contract that upon receiving the NFT, will call updateDistribution. Since totalDepositedXDEFI has not been updated yet, _updateXDEFIBalance will think that the deposit is a reward deposit, not a lock deposit. It will update distributableXDEFI as if there is XDEFI to distribute: [(Code ref])()

uint256 currentDistributableXDEFI = distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI;

So when a user will try to withdraw his funds, the contract will try to reward him with funds that are either aren't his or aren't in the contract at all. Additionally, this line might underflow further on.

Here is a simple POC scenario:

• User 1 locks 10 tokens for 1 day
• Exploiter creates a contract that upon receiving of NFT, will call updateDistribution
• Exploiter uses the contract to deposit 10 tokens for 1 day. Contract calls updateDistribution.
• One day passes
• Exploiter unlocks his tokens
• User 1 can not unlock his tokens now I've created a POC for this scenario. Create LockReenter.sol in the contracts directory, and add the test to XDEFIDistribution.js. POC

Recommended Mitigation Steps

Move the safeMint to the end of lock function. Additionally, you can add a nonReenter modifier on updateDistribution.

Handle

kenzo

Vulnerability details

Token IDs are defined as concatenation of points, total supply + 1. The total supply can decrease when merging. This means that the contract might try to mint a token with an ID which already exists.

Impact

Under specific circumstances, users won't be able to lock or merge tokens.

Proof of Concept

The merge function calls _burn, which decreases the total supply. When generating a new NFT, it's id is determined in _generateNewTokenId using only it's points and current total supply: (Code ref)

    function _generateNewTokenId(uint256 points_) internal view returns (uint256 tokenId_) {
        return (points_ << uint256(128)) + uint128(totalSupply() + 1);
    }

Since the totalSupply will decrease after merge has called _burn, the contract might try to mint an ID which already exists.

Here is one example scenario:

• User 1 locks 10 tokens for 1 day (NFT1)
• User 1 locks 10 tokens for 1 day (NFT2)
• User 2 locks 10 tokens for 2 days (NFT3)
• User 3 locks 10 tokens for 2 days (NFT4)
• One day passes
• User 1 unlocks NFTs 1,2
• User 1 tries to merge NFTs 1,2. At this point the merge function will burn the NFTs and decrease total supply back to 2. It will then try to mint a new NFT, with points that will equal NFT3's points, and since the total supply is 2, the totalSupply+1 portion will be 3 - same as NFT3. Therefore, the mint will revert since the token already exists.

Similarly, the lock function might fail when trying to mint a new token after the supply has decreased.

I've created a test POC for the first example. Add it to XDEFIDistribution.js to see it fails. POC

Recommended Mitigation Steps

Probably do not use totalSupply for the IDs, but save a different counter which never decreases and use it.