XDEFI contest - tqts's results

Handle

tqts

Vulnerability details

Impact

It is possible for attackers to exploit a reentrancy issue in _lock(uint256,uint256,address) that would allow them to lock all funds deposited prior to the attack.

Proof of Concept

The problem lies in the lock(uint256,uint256,address) function, that transfers the XDEFI tokens from the caller and then calls _lock(uint256,uint256,address).

When _lock(uint256,uint256,address) is called, it calls _safeMint(destination_, tokenId_), a function implemented in ERC721.sol. This function, in turn, calls _checkOnERC721Received(address(0), to, tokenId, _data), where to equals destination_, tokenId equals tokenId_, and _data is empty. The code for the _checkOnERC721Received() function is here.

The onERC721Received() callback is called if destination_ is a contract, and this attacker contract can call XDEFIDistribution.updateDistribution() from here.

At this point, totalDepositedXDEFI += amount_; (XDEFIDistribution.sol L265) hasn't been executed yet. This means that IERC20(XDEFI).balanceOf(address(<XDEFIDistribution>)) is greater than totalDepositedXDEFI by exactly the amount_ of XDEFI deposited by the attacker in the lock() function call.

When updateDistribution() calls _updateXDEFIBalance(), newXDEFI will equal amount_ and the _pointsPerUnit variable will be assigned an incorrect value (namely, amount_ * 2**128 / totalUnits). This, in turn, will also affect pointsCorrection of the position when _lock() finishes its execution.

When an attempt is made to unlock any of the tokens minted prior to the attack, the incorrect value of _pointsPerUnit will make _withdrawableGiven() report an inflated value. When _safeTransfer() transfers this value, the posterior call to _updateXDEFIBalance() reverts due to the fact that now IERC20(XDEFI).balanceOf(address(this)) is lower than totalDepositedXDEFI, and the subtraction overflows.

Summary of the attack

• User 1 locks any amount of XDEFI to XDEFIDistribution.
• ...
• User n locks any amount of XDEFI to XDEFIDistribution.
• Attacker deploys AttackerContract, funds it with XDEFI and locks any amount of XDEFI to XDEFIDistribution.
• Users 1 to n are unable to unlock their NFTs.

Short POC contract:

contract AttackerContract {
    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
        XDEFIDistribution(msg.sender).updateDistribution();
        return IERC721Receiver.onERC721Received.selector;
    }

    function attack(address dist, address token) {
        XDEFI(token).approve(dist, XDEFI(token).balanceOf(address(this)));
        XDEFIDistribution(dist).lock(XDEFI(token).balanceOf(address(this)), 0, address(this));
    }
}

Feel free to contact me via Discord DM at @tqts#0820 for more information.

Tools Used

Manual review / Remix

Recommended Mitigation Steps

Add noReenter modifier to updateDistribution()