MerkleDistributor Manual Review Findings

MDR-01M: Improper Execution Guards of claimQuit

Description:

The claimQuit function contains two execution paths one of which will result in locked funds.

Example:

Copy
114function claimQuit() public override {
115    require(block.timestamp <= vestingUntil);
116    Vest storage vest = vests[msg.sender];
117    totalLeft -= (vest.total - vest.claimed);
118
119    uint256 amount = claimable(msg.sender);
120    vest.claimed += amount;
121    uint256 leftToClaim = (vest.total - vest.claimed) / 2;
122    vest.claimed += leftToClaim;
123    uint256 toAirdropHolders = (((vest.total - vest.claimed) * sharePrice) /
124        PRICE_DIVIDEND);
125
126    delete vests[msg.sender];
127    amount = ((leftToClaim + amount) * sharePrice) / PRICE_DIVIDEND;
128
129    _increaseSharePrice(toAirdropHolders);
130
131    IERC20(token).safeTransfer(msg.sender, amount);
132}
133
134function claimable(address account) public view returns (uint256) {
135    Vest storage vest = vests[account];
136    if (vest.total == 0) {
137        return 0;
138    }
139    if (block.timestamp > vestingUntil) {
140        return vest.total - vest.claimed;
141    }
142    return
143        (vest.total * (block.timestamp - vestingStart)) /
144        VESTING -
145        vest.claimed;
146}
147
148function _increaseSharePrice(uint256 amount) internal {
149    if (totalLeft != 0) {
150        sharePrice += (amount * PRICE_DIVIDEND) / totalLeft;
151    }
152}

Recommendation:

We advise the function to not be invoke-able in case the vest.total is equal to zero (preventing re-invocation) and we additionally advise that the case whereby totalLeft == 0 is handled properly by the contract as otherwise the half vest amount that is forfeited will be permanently locked in the contract instead of being re-distributed to other stakers as none will be left.

Alleviation:

Although a require check was properly introduced to prevent zero value claims, logic was not introduced to handle the last valid claim invoking claimQuit in which case half of the forfeited claim amount will be permanently locked in the contract. We advise execution of claimQuit to simply be prohibited for the last member to ensure funds are never locked.

MDR-02M: Potential Share Value Truncation

Description:

The claimVest function offsets a claim of a user by the current sharePrice the calculations of which can truncate, meaning that the final vesting claim may be off by one unit and thus cause the whole claim to fail.

Example:

Copy
102function claimVest() public override {
103    address account = msg.sender;
104    Vest storage vest = vests[account];
105
106    uint256 amount = claimable(account);
107    require(amount != 0, "amount");
108    totalLeft -= amount;
109    vest.claimed += amount;
110    amount = (amount * sharePrice) / PRICE_DIVIDEND;
111    IERC20(token).safeTransfer(account, amount);
112}

Recommendation:

We advise the funds to be transferred out of the contract similarly to SushiSwap's MasterChef whereby the amount being claimed is forced to always match the balance of the contract.

Alleviation:

A _safeTransfer function was introduced in the contract that properly validates whether sufficient balance exists to pay out the claim and in any other case transfers all remaining balance in the contract.