XDEFIDistribution: _unlock function should only be called with tokenId_ parameter

[code423n4]

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_);

[deluca-mike]

While this would save gas, our philosophy is that internal functions should be agnostic of msg and be fully parameterized. it makes it easier to create test harnesses (which call just internal functions).

[Ivshti]

agreed with the sponsor's assessment to not fix this