Gas Optimizations

[code423n4]

#1

Impact

Light DoS of USDC withdrawal system

Proof of Concept

Currently, withdrawals are queued in an array and processed sequentially in a for loop.
However, a malicious user can post unlimited number of tiny (1 wei) withdrawals.
Or, not-malicious user can post multiple withdrawals.
User will receive funds from multiple transfers but it's possible to make only 1 transfer.

USDC transfers are actually expensive due to additional, non-standard SLOADs.

There is more...

Unused array's storage is not freed. I propose usage of mappings, so one can free the memory and get a refund.

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L53-L67

Tools Used

Manual review

Recommended Mitigation Steps

There are 3 ways it can be rewritten:

1st, preserve FIFO order + remove unused storage - multiple calls to the same recipient

2nd, don't preserve FIFO order + remove unused storage - most efficient although unfair property

3nd (BEST), preserve FIFO order + remove unused storage + single call to the same recipient (Aggregate)

1st approach

function withdraw__gas_efficient_1st(uint amount) external {
    burn(amount);
    pendingWithdrawals[end_index] += Withdrawal(msg.sender, aount);
    end_index += 1;
}
function processWithdrawals__gas_efficient_1st() external {
    uint reserve = reserveToken.balanceOf(address(this));
    require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
    uint count = (end_index - start_index);
    uint end = start_index + min(count, maxWithdrawalProcesses);
    uint i;
    for (i = startIndex; i < end; ++i) {
        Withdrawal memory withdrawal = pendingWithdrawals[i];
        if (reserve < withdrawal.amount) {
            break;
        }
        reserveToken.safeTransfer(withdrawal.user, withdrawal.amount);
        reserve -= withdrawal.amount;
        delete pendingWithdrawals[i]; // save gas
        delete pendingWithdrawalsIdx[withdrawal.user]; // save gas
    }
    start_index = i;
}

2nd approach

function withdraw__gas_efficient_2nd(uint amount) external {
    burn(amount);
    uint user_index = pendingWithdrawalsIndicies[msg.sender];
    if (user_index == 0) {
        user_index = end_index++;
        pendingWithdrawalsIndicies[msg.sender] = user_index;
    }
    pendingWithdrawals[user_index] += Withdrawal(msg.sender, aount);
}
function processWithdrawals__gas_efficient_2nd() external {
    uint reserve = reserveToken.balanceOf(address(this));
    require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
    uint count = (end_index - start_index);
    uint end = start_index + min(count, maxWithdrawalProcesses);
    uint i;
    for (i = startIndex; i < end; ++i) {
        Withdrawal memory withdrawal = pendingWithdrawals[i];
        if (reserve < withdrawal.amount) {
            break;
        }
        reserveToken.safeTransfer(withdrawal.user, withdrawal.amount);
        reserve -= withdrawal.amount;
        delete pendingWithdrawals[i]; // save gas
        delete pendingWithdrawalsIdx[withdrawal.user]; // save gas
    }
    start_index = i;
}

3rd approach

function withdraw__gas_efficient_3rd(uint amount) external {
    burn(amount);
    pendingWithdrawals[end_index] += Withdrawal(msg.sender, aount);
    end_index += 1;
}
function processWithdrawals__gas_efficient_3rd() external {
    uint reserve = reserveToken.balanceOf(address(this));
    require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
    uint count = (end_index - start_index);
    uint end = start_index + min(count, maxWithdrawalProcesses);
    uint i;
    // compute
    mapping(address => uint) memory temp_idx;
    Withdrawal[] memory temp_withdrawals
    for (i = startIndex; i < end; ++i) {
        Withdrawal memory withdrawal = pendingWithdrawals[i];
        if (reserve < withdrawal.amount) {
            break;
        }
        uint user_index = temp_idx[withdrawal.user];
        if (user_index == 0) {
            user_index = ++idx;
            temp_withdrawals.push(withdrawal);
        } else {
            temp_withdrawals[user_index - 1].amount += withdrawal.amount;
        }
        reserve -= withdrawal.amount;
        delete pendingWithdrawals[i]; // save gas
        delete pendingWithdrawalsIdx[withdrawal.user]; // save gas
    }
    startIndex = i;

    for (uint j = 0; j < temp_withdrawals.length; ++j) {
        Withdrawal memory withdrawal = temp_withdrawals[j];
        reserveToken.safeTransfer(withdrawal.user, withdrawal.amount); // save gas
    }
}

2

Impact

Excessive SLOAD in a for loop.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L57

Tools Used

Manual review

Recommended Mitigation Steps

Cache array's length in memory

[atvanguard]

Good report.

[moose-code]

Great suggestion and good system knowledge to recognize this in the case of USDC being wildly used.