Hubble contest - csanuragjain's results

Duplicate of https://github.com/code-423n4/2022-02-hubble-findings/issues/119

Duplicate of https://github.com/code-423n4/2022-02-hubble-findings/issues/46

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));
        ...
    }

1. In processWithdrawals function at VUSD.sol#L64, modify i+=1 to ++i

2. In processWithdrawals function at VUSD.sol#L63, use unchecked at reserve -= withdrawal.amount; since we already know that reserve > withdrawal.amount

3. In withdraw function at VUSD.sol#L48, add a check require(amount!=0, "Invalid amount");

4. In mintWithReserve function at VUSD.sol#L43, add below require

require(amount!=0, "Invalid amount");
require(to!=address(0), "Invalid address");

5. getRoundData function at Oracle.sol#L124 is not required. Simply change the if(latestPrice < 0) to while(latestPrice < 0) at Oracle.sol#L117 which will eliminate the need of getRoundData function

function getLatestRoundData(AggregatorV3Interface _aggregator)
        internal
        view
        returns (
            uint80,
            uint256 finalPrice,
            uint256
        )
    {
        (uint80 round, int256 latestPrice, , uint256 latestTimestamp, ) = _aggregator.latestRoundData();
        finalPrice = uint256(latestPrice);
        while (latestPrice < 0) {
            requireEnoughHistory(round);
            (round, finalPrice, latestTimestamp) = getRoundData(_aggregator, round - 1);
        }
        return (round, finalPrice, latestTimestamp);
    }

6. In deposit function at InsuranceFund.sol#L39, add a condition require(amount!=0, "Invalid amount")

7. In withdraw function at InsuranceFund.sol#L62, add a check require( _shares!=0,"Invalid shares");

8. In liquidate function at ClearingHouse.sol#L140, updatePositions function is not required since this is already called in _liquidateMaker and _liquidateTaker function

9. In addMarginFor function at MarginAccount.sol#L149, add new check

require(idx >= supportedCollateral.length,"Invalid index");

10. In removeMargin function at MarginAccount.sol#L177, margin[idx][trader] -= amount.toInt256(); can be unchecked since contract has already checked margin[idx][trader] >= amount.toInt256()

11. In removeMargin function at MarginAccount.sol#L168, add a check require(amount!=0,"Invalid amount");

12. In settleBadDebt function at MarginAccount.sol#L362, modify require(vusdBal < 0, "Nothing to repay"); to require(vusdBal <= 0, "Nothing to repay"); since if vusdBal is 0 then also there is nothing to repay

13. In weightedAndSpotCollateral function at MarginAccount.sol#L524 add a check to see margin[i][trader]==0 as shown below

function weightedAndSpotCollateral(address trader)
        public
        view
        returns (int256 weighted, int256 spot)
    {
        ...

        for (uint i = 0; i < assets.length; i++) {
            _collateral = assets[i];
  if(margin[i][trader]==0){continue}
            int numerator = margin[i][trader] * oracle.getUnderlyingPrice(address(assets[i].token));
            ...
        }
    }

14. In liquidateFlexible function at MarginAccount.sol#L328, before entering loop check if trader can be liquidated

function liquidateFlexible(address trader, uint maxRepay, uint[] calldata idxs) external whenNotPaused {
        clearingHouse.updatePositions(trader); // credits/debits funding
(buffer.status, buffer.repayAble, buffer.incentivePerDollar) = isLiquidatable(trader, false);
if (buffer.status != IMarginAccount.LiquidationStatus.IS_LIQUIDATABLE) {return}

for (uint i = 0; i < idxs.length; i++){
        ...
    }