XDEFI contest - egjlmn1's results

Handle

egjlmn1

Vulnerability details

An attacker can prevent any user from locking assets due to the unsafe id generation for the nfts. https://github.com/XDeFi-tech/xdefi-distribution/blob/master/contracts/XDEFIDistribution.sol#L242

The id is generated by using only the amount deposited, the duration to lock and the totalSiupply of nfts. The first two are easy for the attacker to copy, so all he needs to do is make sure the totalSupply for his nft is the same totalSupply when the user will want to lock his assets. The attacker can do it by merging two tokens.

The attack will go as follow:

1. The attacker sees a user is going to lock assets with duration T and amount X
2. The attacker frontrun and lock 3 times, the first two are not important, he can lock 1 token for 1 second (or the lowest time accepted) and the third lock will be with X tokens and T duration (just like the user) so the totalSupply when locking the third time is initialTotalSupply+2
3. Now the attacker merge the first 2 tokens and thus reduce the totalSupply of nfts by 2 after the _burn in the loop (in line 217), which reduce the totalSupply to initialTotalSupply+1 and increase it by 1 after the _safeMint right after the loop (in line 223) which increase the totalSupply to the initialTotalSupply+2.
4. Now when the user want to lock his assets the totalSupply of nfts will be the same as when the attacker locked his third asset and since both the duration and amount are equal, the tokenID will be equal and the safeMint of the ERC721 will revert.

Impact

A complete DOS attack for any user and any amount (given the attacker as enough money, doesn't need much more than the user) Once the attacker locks all his money he can just wait to unlock it DOS everybody again

Proof of Concept

the _safeMint function increase the totalSupply by 1 and the _burn reduce it by 1. the attacker locks his third asset when totalSupply is +2 so after that the totalSupply is +3, but the merging function will call _burn twice and safeMint once so the totalSupply will be +3-2+1 = +2 and thus the user will fail his lock.

the merge wont fail because the amount to safeMint in the merge is the sum of the two burt tokens

Tools Used

Manual code review

Recommended Mitigation Steps

add another factor for the id generation, like msg.sender (this will prevent dos attacks but users that do the same steps as the attacker supposed to do: lock, lock, lock, merge(1,2) and than try to lock the same amount and duration right after will fail so maybe warn them in the UI)