QA Report

[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 user can post unlimited number of tiny (1 wei) withdrawals. Clearing these withdrawals can be gas consuming and can delay users. (It is gas consuming because USDC transfers require many SSLOAD and SSTORE ops) In general, pushing withdrawal is cheaper than processing withdrawal.

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

Tools Used

Manual review

Recommended Mitigation Steps

Possible solutions: Rewrite withdraw() and processWithdrawals():

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
    }
}

Note: it's also gas optimized:

1. Mapping is preferred instead of array
2. unused mapping's storage is freed (-> refund)
3. Amount of transfer calls is minimized

2

Impact

Chainlink's latestRoundData() might return stale or incorrect results

Proof of Concept

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

There is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Tools Used

Manual review

Recommended Mitigation Steps

Change to

(uint80 roundID, int256 feedPrice, , uint256 timestamp, uint80 answeredInRound) = AggregatorV3Interface(chainLinkAggregatorMap[underlying]).latestRoundData();
require(feedPrice > 0, "Chainlink price <= 0");
require(answeredInRound >= roundID, "Stale price");
require(timestamp != 0, "Round not complete");

[atvanguard]

Duplicate of #119

[moose-code]

Promoting severity of grievance