code423n4 commented 2 years ago

Handle

Jujic

Vulnerability details

Impact

Proof of Concept

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L304-L305

function _unlock(address account_, uint256 tokenId_) internal returns (uint256 amountUnlocked_) {
        // Check that the account is the position NFT owner.
        require(ownerOf(tokenId_) == account_, "NOT_OWNER");

        // Fetch position.
        Position storage position = positionOf[tokenId_];
        uint96 units = position.units;
        uint88 depositedXDEFI = position.depositedXDEFI;
        uint32 expiry = position.expiry;

        // Check that enough time has elapsed in order to unlock.
        require(expiry != uint32(0), "NO_LOCKED_POSITION");
        require(block.timestamp >= uint256(expiry), "CANNOT_UNLOCK");

        // Get the withdrawable amount of XDEFI for the position.
        amountUnlocked_ = _withdrawableGiven(units, depositedXDEFI, position.pointsCorrection);

        // Track deposits.
        totalDepositedXDEFI -= uint256(depositedXDEFI);

        // Burn FDT Position.
        totalUnits -= units;
        delete positionOf[tokenId_];

        emit LockPositionWithdrawn(tokenId_, account_, amountUnlocked_);
    }

Tools Used

Remix

Recommended Mitigation Steps

Change to:

function _unlock(address account_, uint256 tokenId_) internal returns (uint256 amountUnlocked_) {
        // Check that the account is the position NFT owner.
        require(ownerOf(tokenId_) == account_, "NOT_OWNER");

        Position storage position = positionOf[tokenId_];

        uint32 expiry = position.expiry;

        // Check that enough time has elapsed in order to unlock.
        require(expiry != uint32(0), "NO_LOCKED_POSITION");
        require(block.timestamp >= uint256(expiry), "CANNOT_UNLOCK");

       uint96 units = position.units;
       uint88 depositedXDEFI = position.depositedXDEFI;

deluca-mike commented 2 years ago

While this is true, I tested and it actually makes the happy-path more expensive, which is not a good trade-off. Wallets should catch if a transaction will fail, so it's unlikely that it will matter how much gas a reverting transaction costs.

Ivshti commented 2 years ago

agreed with assessment

code-423n4 / 2022-01-xdefi-findings