Brt Dci Contracts - Prodigy

The getPrice function in AggregatorHelper.sol fetches price data from a Chainlink oracle without validating the freshness of the returned data:

function getPrice(address _aggregator, bytes32 _pythPriceFeed, bytes[] calldata _priceUpdateData)
    internal
    returns (int256 price)
{
    if (_pythPriceFeed == bytes32(0)) {
// latestRoundData() returns int256 answer
        (, price,,,) = IChainlinkOracle(_aggregator).latestRoundData();
    } else {
// PythStructs.Price.price is int64
        price =
            IPythAggregator(_aggregator).getLatestEmaPrice{value: msg.value}(_pythPriceFeed, _priceUpdateData).price;
    }
}

The function calls latestRoundData() on the Chainlink oracle but does not check the updatedAt timestamp to ensure the price data is current. This allows stale or outdated price information to be used in critical calculations.

It is recommended to implement a staleness check using the updatedAt value returned by latestRoundData(). Compare it against the current block timestamp and a predefined threshold:

if (_pythPriceFeed == bytes32(0)) {
        (, price,,uint256 updatedAt,) = IChainlinkOracle(_aggregator).latestRoundData();
        require(block.timestamp - updatedAt <= PRICE_FRESHNESS_THRESHOLD, "Stale price data");
    }

SOLVED: Checks have been implemented to ensure price returned by chainlink is correct and not stale.

The PythAggregator contract's updatePrice function, which is called indirectly through getPrice(), does not refund excess ETH sent by users. This function is called in multiple payable functions across Router.sol, Factory.sol, and Vault.sol, allowing users to provide msg.value. The relevant code in PythAggregator.sol is:

function updatePrice(bytes[] calldata _priceUpdateData) public payable {
    uint256 fee = pyth.getUpdateFee(_priceUpdateData);
    pyth.updatePriceFeeds{value: fee}(_priceUpdateData);
}

This implementation transfers the exact fee to the Pyth contract but does not refund any excess ETH sent by the user. The excess ETH remains trapped in the PythAggregator contract.

Users who inadvertently send more ETH than required for price updates will lose their excess funds. This results in a direct financial loss for users and accumulation of unusable ETH in the PythAggregator contract. The severity of this issue is heightened by the fact that it affects multiple core functions of the protocol in Router.sol,Factory.sol and of course Vault.sol

It is recommended to implement a refund mechanism in the updatePrice function to return any excess ETH to the user. Here's an example of how to modify the function:

function updatePrice(bytes[] calldata _priceUpdateData) public payable {
    uint256 fee = pyth.getUpdateFee(_priceUpdateData);
    pyth.updatePriceFeeds{value: fee}(_priceUpdateData);
    uint256 excess = msg.value - fee;
    if (excess > 0) {
        (bool success, ) = msg.sender.call{value: excess}("");
        require(success, "ETH refund failed");
    }
}

SOLVED: Excess fees are now refunded to msg.sender.

The contracts contain multiple instances call of safeTransfer() and safeTransferFrom() calls that do not check if the transfer amount is greater than zero. This issue is present in the following locations:

• In the init() function:

  linkedToken.safeTransferFrom(owner(), address(this), linkedTokenAmount);
  investmentToken.safeTransferFrom(owner(), address(this), investmentTokenAmount + fees);

• In the deposit() function:

  investmentToken.safeTransferFrom(msg.sender, address(this), depositAmount);

• In the withdraw() function:

  linkedToken.safeTransfer(msg.sender, linkedTokenAmount);
  investmentToken.safeTransfer(msg.sender, investmentTokenAmount);

• In the _lpWithdrawInvestmentToken() function:

  investmentToken.safeTransfer(msg.sender, investmentTokenAmount);

• In the _lpWithdrawLinkedToken() function:

  linkedToken.safeTransfer(msg.sender, linkedTokenAmount);

• In the lpWithdraw() function:

  investmentToken.safeTransfer(feeReceiver, fees);
  linkedToken.safeTransfer(feeReceiver, fees);
  investmentToken.safeTransfer(msg.sender, investmentTokenAmount);
  linkedToken.safeTransfer(msg.sender, linkedTokenAmount);

• In the lpCancel() function:

  investmentToken.safeTransferFrom(msg.sender, feeReceiver, cancellationFee);
  linkedToken.safeTransferFrom(msg.sender, feeReceiver, cancellationFee);

These transfers are executed without verifying if the amount is greater than zero. Some ERC20 tokens, such as older implementations or non-standard tokens, revert when a transfer amount of zero is attempted. The absence of a zero amount check can cause these transfers to fail and revert the entire transaction.

Refs :

• Solodit-Caviar

• Solodit-Concur

To address this, it is recommended to implement zero-amount checks before all token transfers. Add a condition to skip the transfer if the amount is zero. Here's an example of how to modify the code:

if (amount > 0) {
    token.safeTransfer(receiver, amount);
}

SOLVED: Checks for non 0 amounts have been added to the needed functions.

The PythAggregator contract does not perform input validation on the price, conf, and expo values returned from the called price feed, which can lead to the contract accepting invalid or untrusted prices. Those values should be checked as clearly stated in the official documentation.

It is recommended that the contract revert the transaction here if one of the following conditions is triggered:

• price <= 0

• expo < -18

• conf > 0 && (price / int64(conf) < MIN_CONF_RATIO for a given MIN_CONF_RATIO

SOLVED: Pyth price now have checks about price,expo and conf.

The Factory.sol contract allows unrestricted vault creation, contradicting the documented requirement for Liquidity Providers (LPs). The createVault() function lacks access control:

function createVault(DCIStructs.CreateVaultParams calldata _createVaultParams, bytes[] calldata _priceUpdateData)
    external
    payable
    whenNotPaused
    returns (address newVault)
{
// Function implementation
}

This implementation permits any address to create vaults without authorization checks.

It is recommended to either modify the documentation or implement strict access control for vault creation.

SOLVED: Documentation has been updated.

In the AggregatorHelper.sol contract, several unsafe casting operations are performed without using SafeCast library. This could potentially lead to unexpected overflows or underflows in certain edge cases.

The following unsafe casting operations were identified:

1. int32(-1 * int8(IChainlinkOracle(_aggregator).decimals()))

2. uint256(uint64(price)) * 10 ** uint32(targetDecimals - priceDecimals)

3. uint256(uint64(price)) / 10 ** uint32(priceDecimals - targetDecimals)

These operations involve casting between different integer types (int8, int32, uint64, uint256) without using SafeCast library to prevent potential overflows or underflows.

Unsafe casting could potentially lead to unexpected behavior or vulnerabilities in edge cases or future modifications of the contract.

It is recommended to consider using OpenZeppelin's SafeCast library for all casting operations to prevent potential overflows or underflows. This is a best practice that enhances the overall robustness and security of the contract.

SOLVED: SafeCast is now used everywhere.

The Vault contract calculates fees during the init() function call instead of at contract creation. This delay allows for a discrepancy between the price at contract creation and the price at initialization.

Relevant code snippets:

// In constructor
constructor(DCIStructs.VaultParams memory _vaultParams, DCIStructs.FeeParams memory _feeParams)
    Ownable(_vaultParams.owner)
{
    // ... other initializations ...
    oraclePriceAtCreation = _feeParams.oraclePriceAtCreation;
}

// In init() function
function init() external onlyOwner {
    require(!isInitialized, "Vault: contract instance has already been initialized");
    require(block.timestamp <= depositDeadline, "Vault: deposit period has ended");

    // ... other logic ...

    uint256 fees = _calculateFees(quantity);
    
    // ... fee transfer logic ...

    isInitialized = true;
    emit Initialize();
}

// Fee calculation function
function _calculateFees(uint256 amount) internal view returns (uint256 fees) {
    if (isBuyLow) {
        fees = amount * listingFeeRate / 1e18;
    } else {
        fees = amount * listingFeeRate * SafeCast.toUint256(oraclePriceAtCreation) / 1e36;
    }
}

This leads to incorrect fee calculations. A malicious vault creator could exploit this by:

1. Deploying the contract when prices are high.

2. Waiting for prices to drop.

3. Calling init() when prices are low, resulting in lower fees.

It is recommended to calculate and store the fees in the constructor to ensure they are based on the price at contract creation.

SOLVED: Fees are still computed in constructor but added to linkedTokenAmount or investmentTokenAmount so the problem is solved.

The lpWithdraw function in the Vault contract violates the Checks-Effects-Interactions (CEI) pattern. The lpWithdrawn state variable is set to true after external interactions are performed, allowing for potential reentrancy attacks. The problematic code is as follows:

function lpWithdraw(bytes[] calldata priceUpdateData) external payable onlyOwner initialized {
    if (state == 0) execute(priceUpdateData);
    require(block.timestamp >= withdrawDate, "Vault: withdraw date has not passed yet");
    require(!lpWithdrawn, "Vault: LP has withdrawn already");

    uint256 linkedTokenAmount;
    uint256 investmentTokenAmount;

    if (depositTotal != 0) {
        uint256 fees = _calculateFees(depositTotal);
        (isBuyLow ? investmentToken : linkedToken).safeTransfer(feeReceiver, fees);
    }

    if (state == 1) {
        investmentTokenAmount = IERC20(investmentToken).balanceOf(address(this));
        investmentToken.safeTransfer(msg.sender, investmentTokenAmount);
        _lpWithdrawLinkedToken(false);
    } else {
        linkedTokenAmount = IERC20(linkedToken).balanceOf(address(this));
        linkedToken.safeTransfer(msg.sender, linkedTokenAmount);
        _lpWithdrawInvestmentToken(false);
    }

    //E @audit CEI NOT RESPECTED
    lpWithdrawn = true;

    emit LpWithdraw(linkedTokenAmount, investmentTokenAmount);
}

To mitigate this, it is recommended to implement the Checks-Effects-Interactions pattern by updating the lpWithdrawn state variable before performing any external calls. Restructure the function as follows:

function lpWithdraw(bytes[] calldata priceUpdateData) external payable onlyOwner initialized {
    if (state == 0) execute(priceUpdateData);
    require(block.timestamp >= withdrawDate, "Vault: withdraw date has not passed yet");
    require(!lpWithdrawn, "Vault: LP has withdrawn already");

    lpWithdrawn = true;

    // ... // 

    emit LpWithdraw(linkedTokenAmount, investmentTokenAmount);
}

SOLVED: lpWithdrawn = true; is now set right after the require statement.

It is best practice to set the visibility of state variables and constants explicitly.

bool hasEarlyWithdrawn = false;
bool lpWithdrawn = false;

Consider explicitly setting the visibility of all state variables and constants.

SOLVED: Each variable has now a visibility set.

Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

mapping(bytes32 => uint8) public tokenPairDecimals;
mapping(address => uint256) public balances;

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

mapping(bytes32 tokenPair => uint8 decimals) public tokenPairDecimals;
mapping(address user => uint256 balance) public balances;

SOLVED: Mappings are now using named arguments.