XDEFI contest - PierrickGT's results

Handle

PierrickGT

Vulnerability details

Impact

In lockWithPermit, we use the same code to transfer XDEFI and lock the position than in lock. We can create an internal function to reuse this code and avoid duplication.

Proof of Concept

Create an internal function called _lockPosition that will transfer XDEFI and lock the position. This function will be called in lock and lockWithPermit.

Recommended Mitigation Steps

The following change is recommended.

function _lockPosition(uint256 amount_, uint256 duration_, address destination_) internal returns (uint256 tokenId_) {
    // Lock the XDEFI in the contract.
    SafeERC20.safeTransferFrom(IERC20(XDEFI), msg.sender, address(this), amount_);

    // Handle the lock position creation and get the tokenId of the locked position.
    return _lock(amount_, duration_, destination_);
}

function lock(uint256 amount_, uint256 duration_, address destination_) external noReenter returns (uint256 tokenId_) {
    return _lockPosition(amount_, duration_, destination_);
}

function lockWithPermit(uint256 amount_, uint256 duration_, address destination_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_) external noReenter returns (uint256 tokenId_) {
    // Approve this contract for the amount, using the provided signature.
    IEIP2612(XDEFI).permit(msg.sender, address(this), amount_, deadline_, v_, r_, s_);

    return _lockPosition(amount_, duration_, destination_);
}

Handle

PierrickGT

Vulnerability details

Impact

We pass account_ to the _unlock function but this one is always called with msg.sender as first parameter. We can simplify the code by removing the account_ parameter and calling msg.sender inside the function instead.

Proof of Concept

Remove the account_ parameter from _unlock and _unlockBatch to only use msg.sender inside the _unlock function.

Recommended Mitigation Steps

The following changes are recommended.

For the _unlock function:

function _unlock(uint256 tokenId_) internal returns (uint256 amountUnlocked_) {
    // Check that the account is the position NFT owner.
    require(ownerOf(tokenId_) == msg.sender, "NOT_OWNER");
    ...
    emit LockPositionWithdrawn(tokenId_, msg.sender, amountUnlocked_); // On line 317
}

On line 112:

amountUnlocked_ = _unlock(tokenId_);

On line 133:

amountUnlocked_ = _unlock(tokenId_);

On line 326:

amountUnlocked_ += _unlock(tokenIds_[i]);

For the _unlockBatch function:

function _unlockBatch(uint256[] memory tokenIds_) internal returns (uint256 amountUnlocked_) {

On line 167:

amountUnlocked_ = _unlockBatch(tokenIds_);

On line 188:

amountUnlocked_ = _unlockBatch(tokenIds_);