Hubble contest - throttle's results

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");

Lines of code

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

Vulnerability details

Impact

DoS of USDC withdrawal system

Proof of Concept

Currently, withdrawals are queued in an array and processed sequentially in a for loop. However, a safeTransfer() to USDC blacklisted user will fail. It will also brick the withdrawal system because the blacklisted user is never cleared.

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

Tools Used

Manual review

Recommended Mitigation Steps

Possible solutions: 1st solution: Implement 2-step withdrawals: - In a for loop, increase the user's amount that can be safely withdrawn. - A user himself withdraws his balance

2st solution: Skip blacklisted users in a processWithdrawals loop

#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