Hubble contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L24-L35

Vulnerability details

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L24-L35

function getUnderlyingPrice(address underlying)
    virtual
    external
    view
    returns(int256 answer)
{
    if (stablePrice[underlying] != 0) {
        return stablePrice[underlying];
    }
    (,answer,,,) = AggregatorV3Interface(chainLinkAggregatorMap[underlying]).latestRoundData();
    answer /= 100;
}

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L64-L69

(uint80 round, uint256 latestPrice, uint256 latestTimestamp) = getLatestRoundData(aggregator);
uint256 baseTimestamp = _blockTimestamp() - intervalInSeconds;
// if latest updated timestamp is earlier than target timestamp, return the latest price.
if (latestTimestamp < baseTimestamp || round == 0) {
    return formatPrice(latestPrice);
}

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L460-L468

function _getLiquidationInfo(address trader, uint idx) internal view returns (LiquidationBuffer memory buffer) {
    require(idx > VUSD_IDX && idx < supportedCollateral.length, "collateral not seizable");
    (buffer.status, buffer.repayAble, buffer.incentivePerDollar) = isLiquidatable(trader, false);
    if (buffer.status == IMarginAccount.LiquidationStatus.IS_LIQUIDATABLE) {
        Collateral memory coll = supportedCollateral[idx];
        buffer.priceCollateral = oracle.getUnderlyingPrice(address(coll.token)).toUint256();
        buffer.decimals = coll.decimals;
    }
}

On Oracle.sol, we are using Chainlink's latestRoundData API, but 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

The result of latestRoundData API will be used for MarginAccount.sol#liquidateExactRepay(), therefore, a stale price from Chainlink can lead to loss of funds to end-users.

Recommendation

Consider adding missing checks for stale data.

For example:

(uint80 roundID ,answer,, uint256 timestamp, uint80 answeredInRound) = AggregatorV3Interface(chainLinkAggregatorMap[underlying]).latestRoundData();

require(answer > 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/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L373-L380

Vulnerability details

In MarginAccount.sol#settleBadDebt(), the collateral assets will be seized and transferred to the insuranceFund contract.

However, there is no way for the liquidity providers of the insuranceFund to get back the collateral assets.

In the current implementation, these collateral assets seized during settleBadDebt() will be frozen in the contract, in essence. They belong to the liquidity providers and they should be able to retrieve them.

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L373-L380

for (uint i = 1 /* skip vusd */; i < assets.length; i++) {
    int amount = margin[i][trader];
    if (amount > 0) {
        margin[i][trader] = 0;
        assets[i].token.safeTransfer(address(insuranceFund), amount.toUint256());
        seized[i] = amount.toUint256();
    }
}

Recommendation

Consider adding a new method for the liquidity providers to claim certain collateral assets proportionally to the shares they held.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/InsuranceFund.sol#L116-L119

Vulnerability details

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/InsuranceFund.sol#L116-L119

function syncDeps(IRegistry _registry) public onlyGovernance {
    vusd = IERC20(_registry.vusd());
    marginAccount = _registry.marginAccount();
}

The Governance address can call InsuranceFund.sol#syncDeps() to change the contract address of vusd anytime.

However, since the tx to set a new address for vusd can get in between users' txs to deposit and withdraw, in some edge cases, it can result in users' loss of funds.

PoC

1. Alice deposited 1,000,000 VUSD to InsuranceFund;
2. Gov called syncDeps() and set vusd to the address of VUSDv2;
3. Alice called withdraw() with all the shares and get back 0 VUSDv2.

As a result, Alice suffered a fund loss of 1,000,000 VUSD.

Recommendation

1. Consider making vusd unchangeable;
2. If a possible migration of vusd must be considered, consider changing the syncDeps() to:

function syncDeps(IRegistry _registry) public onlyGovernance {
    uint _balance = balance();
    vusd = IERC20(_registry.vusd());
    require(balance() >= _balance);
    marginAccount = _registry.marginAccount();
}

Lines of code

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

Vulnerability details

Use of Upgradeable Proxy Contract Structure allows the logic of the contract to be arbitrarily changed.

This allows the proxy admin to perform malicious actions e.g., taking funds from users' wallets up to the allowance limit.

This action can be performed by the malicious/compromised proxy admin without any restriction.

Considering that the purpose of this particular contract is for a token, we believe the smart contract should not be structured as an upgradeable contract.

PoC

Given:

• reserveToken: USDC

1. Alice approve VUSD spending her USDC and mintWithReserve() VUSD;
2. Bob approve VUSD spending his USDC and mintWithReserve() VUSD;
3. A malicious/compromised proxy admin can call upgradeToAndCall() on the proxy contract and set a malicious contract as newImplementation and stolen all the WBTC in Alice's and Bob's wallets;

Severity

A smart contract being structured as an upgradeable contract alone is not usually considered as a high severity risk. But given the severe impact (funds in users' wallets got stolen), with the PoC above, once the proxy admin is compromised, users' funds can be at risk. Therefore, we mark it as a High severity issue.

Recommendation

1. Consider making the contract non-upgradeable.
2. Consider using a non-upgradable contract to hold users' allowances.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Duplicated assert statements

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/curve-v2/Swap.vy#L666-L671

@external
@nonreentrant('lock')
def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256) -> (uint256):
    assert msg.sender == self.amm, 'VAMM: OnlyAMM'
    assert not self.is_killed  # dev: the pool is killed
    assert msg.sender == self.amm

L669 and L671 are duplicated.

[S] Outdated versions of OpenZeppelin library

Outdated versions of OpenZeppelin library are used.

It's a best practice to use the latest version of libraries.

New versions of OpenZeppelin libraries can be more gas effeicant.

For exmaple:

ERC20Upgradeable.sol in @openzeppelin/contracts-upgradeable@4.3.2:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.3.2/contracts/token/ERC20/ERC20Upgradeable.sol#L155-L169

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);

        uint256 currentAllowance = _allowances[sender][_msgSender()];
        require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
        unchecked {
            _approve(sender, _msgSender(), currentAllowance - amount);
        }

        return true;
    }

A gas optimization upgrade has been added to @openzeppelin/contracts@4.5.0:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.0/contracts/token/ERC20/ERC20Upgradeable.sol#L163-L172

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual override returns (bool) {
        address spender = _msgSender();
        _spendAllowance(from, spender, amount);
        _transfer(from, to, amount);
        return true;
    }

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.5.0/contracts/token/ERC20/ERC20Upgradeable.sol#L335-L347

    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC20: insufficient allowance");
            unchecked {
                _approve(owner, spender, currentAllowance - amount);
            }
        }
    }

• reduce allowance before triggering transfer. (#3056)
• do not update allowance on transferFrom when allowance is type(uint256).max. (#3085)
• cache _msgSender (#3167)

[S] Avoid unnecessary storage reads in for loops can save gas

For the storage variables that will be accessed multiple times, especially in loops, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

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

    function processWithdrawals() external {
        uint reserve = reserveToken.balanceOf(address(this));
        require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
        uint i = start;
        while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) {
            Withdrawal memory withdrawal = withdrawals[i];
            if (reserve < withdrawal.amount) {
                break;
            }
            reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
            reserve -= withdrawal.amount;
            i += 1;
        }
        start = i;
    }

start and maxWithdrawalProcesses can be cached.

[M] Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L572-L579

    function _transferInVusd(address from, uint amount) internal {
        IERC20(address(vusd)).safeTransferFrom(from, address(this), amount);
        if (credit > 0) {
            uint toBurn = Math.min(vusd.balanceOf(address(this)), credit);
            credit -= toBurn;
            vusd.burn(toBurn);
        }
    }

credit -= toBurn will never overflow.

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L581-L593

    function _transferOutVusd(address recipient, uint amount) internal {
        uint bal = vusd.balanceOf(address(this));
        if (bal < amount) {
            // Say there are 2 traders, Alice and Bob.
            // Alice has a profitable position and realizes their PnL in form of vusd margin.
            // But bob has not yet realized their -ve PnL.
            // In that case we'll take a credit from vusd contract, which will eventually be returned when Bob pays their debt back.
            uint _credit = amount - bal;
            credit += _credit;
            vusd.mint(address(this), _credit);
        }
        IERC20(address(vusd)).safeTransfer(recipient, amount);
    }

uint _credit = amount - bal will never overflow.

[S] Using immutable variable can save gas

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccountHelper.sol#L15-L26

    IMarginAccount marginAccount;
    VUSD vusd;
    IERC20 public reserveToken;

    constructor(address _marginAccount, address _vusd) {
        marginAccount = IMarginAccount(_marginAccount);
        vusd = VUSD(_vusd);
        reserveToken = vusd.reserveToken();

        reserveToken.safeApprove(address(_vusd), type(uint).max);
        IERC20(_vusd).safeApprove(address(_marginAccount), type(uint).max);
    }

Considering that marginAccount, vusd and reserveToken will never change, changing them to immutable variables instead of storages variable can save gas.