QA Report

[code423n4]

Withdrawals can stuck

1. Consider processWithdrawals function at VUSD.sol#L53
2. Assume contract has balance of 100
3. Withdrawal request are 101,10,20,30
4. processWithdrawals function is called
5. Function fail since contract does not have 101 balance
6. But due to this the remaining transaction also get blocked 10,20,30 for which contract had sufficient balance

Recommendation: If contract does not have balance for particular withdrawal instance, keep that in pending object and try to complete the remaining ones

Withdraw timelock missing

1. The withdraw function at VUSD.sol#L48 is not placing any timelock which means user can call withdraw function frequently and push them into withdrawals object. This can delay other user withdrawal which are placed in long queue back and since processWithdrawals can only process maxWithdrawalProcesses at one run, other user withdrawal may delay

Recommendation: There should be a timelock after which withdraw can be called again otherwise this can be called repeatedly for small amounts If user has requested withdraw then he should only be able to call this function again after x timestamp

Shares can give lower value

1. Consider withdraw function at InsuranceFund.sol#L62

2. if some big bad debt comes (seizeBadDebt at MarginAccount.sol#L368) then settlePendingObligation function which is called at withdraw function will consume most contract balance. This will reduce amount in balance()

3. Since withdraw amount is directly proportional to balance (uint amount = balance() * _shares / totalSupply();) so same shares will give less amount

Missing Oracle price checks

1. In getLatestRoundData function at Oracle.sol#L115, there is no check to see if returned price of _aggregator.latestRoundData() is not stale. More details at https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Recommendation: Modify the function as below:

(uint80 round, int256 latestPrice, , uint256 latestTimestamp, uint80 answeredInRound) = _aggregator.latestRoundData();
require(feedPrice > 0, "Chainlink price <= 0"); 
require(answeredInRound >= round, "Stale price");
require(latestTimestamp != 0, "Round not complete");

Input validation missing

1. It was observed that price can be set to 0 in setStablePrice function at Oracle.sol#L169. This is incorrect since the contract checks stablePrice[underlying] != 0 in other functions like getUnderlyingPrice.

Recommendation: Add below check

function setStablePrice(address underlying, int256 price) external onlyGovernance {
require(price!=0,"Invalid price")
        requireNonEmptyAddress(underlying);
        stablePrice[underlying] = price;
    }

Incorrect condition can give incorrect price

1. The getUnderlyingTwapPrice function at Oracle.sol#L67 is returning latestPrice when latestTimestamp < baseTimestamp.

2. Else it would goto previous rounds

3. This is incorrect. This function should return latestPrice when latestTimestamp = baseTimestamp

Recommendation: Modify the check like below

if (latestTimestamp <= baseTimestamp || round == 0) {
            return formatPrice(latestPrice);
        }

Zero address checks are missing

1. For all address arguments at constructor of Registry.sol#L12. Add below require

require(_oracle!=address(0), "Invalid address");
require(_clearingHouse!=address(0), "Invalid address");
require(_insuranceFund!=address(0), "Invalid address");
require(_marginAccount!=address(0), "Invalid address");
require(_vusd!=address(0), "Invalid address");

2. Initialize function at AMM.sol#L93, add below require

require(_registry!=address(0), "Invalid address");
require(_underlyingAsset!=address(0), "Invalid address");
require(_vamm!=address(0), "Invalid address");

3. Initialize function at ClearingHouse.sol#L35, add below require

require(_insuranceFund!=address(0), "Invalid address");
require(_marginAccount!=address(0), "Invalid address");
require(_vusd!=address(0), "Invalid address");

4. In getUnderlyingPrice function, check aggregator is non empty address

function getUnderlyingPrice(address underlying)
        virtual
        external
        view
        returns(int256 answer)
    {
AggregatorV3Interface aggregator = AggregatorV3Interface(chainLinkAggregatorMap[underlying]);
requireNonEmptyAddress(address(aggregator));
        ...
    }

[atvanguard]

Good report that includes duplicates from other severity 2 and 3 issues.

[moose-code]

"There should be a timelock after which withdraw can be called again otherwise this can be called repeatedly for small amounts" - you could just use a sybil type attack which makes a timelock not effective to defend against what you want to defend against.

[moose-code]

Looks like some things in here should be upgraded to beyond low. Will circle back after medium and high severity issue reviews