Rewards can be stolen by re-entering into updateDistribution

[code423n4]

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.

[deluca-mike]

Valid, and duplicate of #25