Platform: Code4rena
Start Date: 02/04/2021
Pot Size: $50,000 USDC
Total HM: 20
Participants: 5
Period: 6 days
Judge: Zak Cole
Total Solo HM: 19
Id: 3
League: ETH
Rank: 4/5
Findings: 3
Award: $4,041.86
🌟 Selected for report: 3
🚀 Solo Findings: 1
s1m0
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.
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.
Manual analysis
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?
#0 - werg
2021-04-08T21:19:06Z
Exactly. we need to initialize lastUdpatedDay. Thanks for the report!
🌟 Selected for report: s1m0
s1m0
Functions like setLeveragePercent and setLiquidationThresholdPercent for both IsolatedMarginTrading (https://github.com/code-423n4/marginswap/blob/main/contracts/IsolatedMarginTrading.sol) and CrossMarginTrading (https://github.com/code-423n4/marginswap/blob/main/contracts/CrossMarginTrading.sol) should be put behind a timelock because they would give more trust to users. Now the owner could call them whenever he wants and a position could become liquidable from a block to the other.
Manual analysis.
Add a timelock to setter functions of critical variables.
#0 - werg
2021-04-08T20:36:05Z
Timelock will be handled by governance
#1 - zscole
2021-04-22T15:30:03Z
Maintaining submission rating of 2 (Med Risk) because this presents a vulnerability at the time of review.
s1m0
0x9b3E9e3E4a174d59279FC7cd268e035992412384
The owner can initialize an already initialized tranche by calling setTranche https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L78 with 0 as share argument and then calling initTranche https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L101 bypassing the check require(tm.rewardShare == 0, "Tranche already initialized");
Check share != 0 for setTrancheShare and initTranche
The state of the system would become not correct by inflating the allTranches variable and it would raise the gas cost for calling withdrawReward
Manual analysis
Assuming the 1 tranche is initialized.
#0 - zscole
2021-04-22T15:22:28Z
Duplicate of #35
🌟 Selected for report: s1m0
s1m0
0x9b3E9e3E4a174d59279FC7cd268e035992412384
The constructor of IncentiveDistribution https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L32 take as argument the address of MFI token but it doesn't check that is != address(0). Not worth an issue alone but IncentiveDistribution imports IERC20.sol and it never use it.
In case the address(0) is passed as arguement the withdrawReward woul fail https://github.com/code-423n4/marginswap/blob/main/contracts/IncentiveDistribution.sol#L261 and due to the fact that MFI is immutable the only solution would be to redeploy the contract meanwhile losing trust from the users.
Deploy IncentiveDistribution with 0 as _MFI argument and then call withdrawReward.
Manual analysis
Check _MFI != address(0)
🌟 Selected for report: s1m0
s1m0
0x9b3E9e3E4a174d59279FC7cd268e035992412384
When changing state variables events are not emitted. PriceAware (https://github.com/code-423n4/marginswap/blob/main/contracts/PriceAware.sol):
The events emitted by MarginRouter (https://github.com/code-423n4/marginswap/blob/main/contracts/MarginRouter.sol) don't have indexed parameter.
Manual analysis
The system doesn't record historical state changes.
For set... function emit events with old and new value. For initTranche, event InitTranche(uint256 tranche, uint256 share) For activateIssuer, event ActivateIssuer(address issuer, address token) For deactivateIssuer, event DeactivateIssuer(address issuer) For events emitted by MarginRouter i would index the trader address to make it filterable.
#0 - werg
2021-04-08T21:24:38Z
We may sprinkle in a few more events before launch, but in the interest of gas savings we try not to emit events for state that can be queried using view functions.
#1 - zscole
2021-04-22T15:27:07Z
Reducing this from submitted rating of 2 (Med Risk) to 1 (Low Risk) since it presents no immediate risk to the security of the system, but could have implications on overall functionality.