XDEFI contest - onewayfunction's results

Handle

onewayfunction

Vulnerability details

Impact

Anyone can steal XDEFI from the XDEFIDistribution contract, thereby making the contract insolvent. In the process, they also make the updateDistribution() function uncallable -- and thus make the value of _pointsPerUnit unchangeable.

This comes with some risk to the attacker. The size of the risk is directly proportional to the smallest allowable lock duration.

Proof of Concept

When a user calls the lock() function, XDEFI tokens are transferred into the XDEFIDistribution contract, and then _lock() is called.

The _lock() function does two important accounting-related things: (a) it records the amount of XDEFI tokens transferred into the contract (by increasing totalDepositedXDEFI) and (b) it records the user's Position (which depends on the value of _pointsPerUnit).

However, before the _lock() function performs (a) and (b), it first hands over control of the flow of execution to the attacker (caller). It does this during the call to _safeMint(). In particular, the _safeMint() function calls _checkOnERC721Received, which makes an unsafe call to the receiver of the newly minted token. Note that this unsafe external call is not a staticcall, and so it can mutate state.

This unsafe external call happens after the contract has received the user's XDEFI tokens, but before any other accounting is done (e.g., before totalDepositedXDEFI has been updated). So an attacker can set the destination_ address of their call to the lock() function to be a malicious contract with a malicious onERC721Received function. The malicious onERC721Received would call the updateDistribution() function (which does not have the noReenter modifier). When this happens, the updateDistribution() function calls the _updateXDEFIBalance() function, which will incorrectly add the attacker's XDEFI deposit to the distributableXDEFI variable.

This results in the _pointsPerUnit being incorrectly increased, which means anyone who locked before this attacking tx will be able to withdraw more XDEFI tokens than they should be able to. This also means the contract is insolvent, and the last user(s) trying to unlock will not be able to get all their XDEFI back. It also means that all future calls to updateDistribution() will revert because the _updateXDEFIBalance function will return a negative number, which will cause L147 to revert when trying to cast to a uint256.

Since the updateDistribution() function is the only function that can update the _pointsPerUnit variable, this means the _pointsPerUnit variable is effectively frozen. (It's possible to unfreeze all this by having an altruist send a bunch of XDEFI directly to the XDEFIDistribution contract and then calling the updateDistribution function, but I think that's unlikely to occur, as they get nothing in return for their sacrifice).

Tl;Dr: The attacker first makes one or more valid locks. When their duration has passed, they perform the above "malicious lock" attack, which lets their previous locks be unlocked to withdraw more XDEFI than they deserve. After the "malicious lock's" duration has passed, the attacker can withdraw their remaining lock (which will just result in them withdrawing the same amount of XDEFI that they put in during the "malicious lock").

The risk to the attacker is that after their malicious lock, the contract is insolvent. So if there is a run on the contract, the contract may be drained of all XDEFI before the attacker's "malicious lock" can be unlocked. This risk is proportional to the minimum allowable lock duration.

Recommended Mitigation Steps

Instead of using the _safeMint function in the _lock and merge functions, consider using the basic _mint function, which doesn't make any unsafe external calls.

Handle

onewayfunction

Vulnerability details

Impact

More than one historical lock-position may be represented by a single tokenId, violating the "uniqueness" property claimed by the xdefi-distribution repo's README.md.

Proof of Concept

The README.md says:

The NFT's score is embedded in the tokenId, so the chain enforces it (first/leftmost 128 bits is the score, last/rightmost 128 bits is a sequential identifier, for uniqueness).

The _generateNewTokenId function use1 the totalSupply() in an attempt to enforce this uniqueness property.

However, during a merge, two or more NFTs are burned, while only one new NFT is minted is minted. So during a merge, the totalSupply() decreases. This means it is possible for future token to have the same tokenId as a token that has previously been burned, violating the "uniqueness" property. (This can even be done intentionally by with a prudent choice of amount and duration when calling lock()).

I don't know how important (or not) the uniqueness assumptions are for your off-chain code, so I rated this a "medium". Feel free to adjust the severity however you see fit, of course.

Recommended Mitigation Steps

If uniqueness of the tokenId matters (for example, for off-chain services), consider modifying the _generateNewTokenId function so that is uses a nonce (say, uint128 totalEverMinted) instead of the totalSupply(). The nonce can be increased during every mint by hooking into the _beforeTokenTransfer or _afterTokenTransfer hooks of the _mint function.