Handle

s1m0

Vulnerability details

Impact

The withdrawReward (https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L224) fails due to the loop at https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L269. From my testing the dayDiff would be 18724 and with a gasLimit of 9500000 it stops at iteration 270 due to the fact that lastUpdatedDay is not initialized so is 0. Other than that it could run out of gas also for the loop of allTranches (https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L281) because it's an unbounded array.

Proof of Concept

This is a test with hardhat.

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

describe("MarginSwap", function() { let IncentiveDistribution, incentiveDistribution; let owner;

before(async function() {
    [owner] = await ethers.getSigners();
    IncentiveDistribution = await ethers.getContractFactory("IncentiveDistribution");
    incentiveDistribution = await IncentiveDistribution.deploy(ethers.constants.AddressZero, 2);
});

it("withdrawReward", async function() {
    await incentiveDistribution.initTranche(1, 23);
    await incentiveDistribution.addToClaimAmount(1, owner.address, 324234);
    await incentiveDistribution.withdrawReward([1], {gasLimit: 9500000});
});

});

Note: from the IncentiveDistribution contract i removed the inheritance of RoleAware and Ownable for convenience of testing and added some print with console.log() to check where it stops.

Tools Used

Manual analysis

Recommended Mitigation Steps

I'm not sure of the logic behind the shrinking of the daily distribution but i think that maybe you just missed to initialize the lastUpdatedDay to the day of deployment? If that's the case it resolves partially the problem because allTranches is theoretically unbounded even though only the owner can add element to it and you should do deeply testing to understand how many elements it can have until it run out of gas. I read the comment that says you tried to shift the gas to the withdrawal people maybe you went too further and is it worth rethinking the design?

code-423n4 / 2021-04-marginswap-findings