Unbounded for loops allows an attacker to freeze users' funds

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-bootfinance/blob/f102ee73eb320532c5a7c1e833f225c479577e39/vesting/contracts/Vesting.sol#L193-L205

function claim() external whenNotPaused nonReentrant {
    require(benRevocable[msg.sender][1] == false, 'Account must not already be revoked.');
    uint256 amount = _claimableAmount(msg.sender).sub(benClaimed[msg.sender]);
    require(amount > 0, "Claimable amount must be positive");
    require(amount <= benTotal[msg.sender], "Cannot withdraw more than total vested amount");

    // transfer from SC
    benClaimed[msg.sender] = benClaimed[msg.sender].add(amount);
    totalClaimedAmount = totalClaimedAmount.add(amount);
    vestingToken.safeTransfer(msg.sender, amount);

    emit TokenClaimed(msg.sender, amount, block.timestamp);
}

At L195, function claim() will call function _claimableAmount(), which includes an unbounded for loop.

https://github.com/code-423n4/2021-11-bootfinance/blob/f102ee73eb320532c5a7c1e833f225c479577e39/vesting/contracts/Vesting.sol#L162-L188

function _claimableAmount(address _addr) private returns (uint256) {
    uint256 completely_vested = 0;
    uint256 partial_sum = 0;
    uint256 inc = 0;

    // iterate across all the vestings
    // & check if the releaseTimestamp is elapsed
    // then, add all the amounts as claimable amount
    for (uint256 i = benVestingIndex[_addr]; i < timelocks[_addr].length; i++) {
        if (block.timestamp >= timelocks[_addr][i].releaseTimestamp) {
            inc += 1;
            completely_vested = completely_vested.add(timelocks[_addr][i].amount);
        }
        else {
            uint256 iTimeStamp = timelocks[_addr][i].releaseTimestamp.sub(unixYear);
            uint256 claimable = block.timestamp.sub(iTimeStamp).mul(timelocks[_addr][i].amount).div(unixYear);
            partial_sum = partial_sum.add(claimable);
        }
    }

    benVestingIndex[_addr] +=inc;
    benVested[_addr][0] = benVested[_addr][0].add(completely_vested);
    benVested[_addr][1] = partial_sum;
    uint256 s = benVested[_addr][0].add(partial_sum);
    assert(s <= benTotal[_addr]);
    return s;
}

An attacker can call function vest() and add a lot of very small amounts of new vestings to the user, if the length of the user's vestings is large enough, the gas cost of function claim() can exceed the block limit, making it impossible for the user to claim.

Essentially, this allows an attacker to freeze (or burn, considering that the contract is not upgradable) the unclaimed funds of an arbitrary user.

Recommendation

Consider allowing users to claim() a specific range of vestings indexes.

[chickenpie347]

Duplicate of #120