Missing zero address validation for Ether or token transfers.

Summary

The function BaseStrategyVault.redeemFromNotional(address,address,uint256,uint256,uint256,bytes) in the file BaseStrategyVault.sol is a function that lacks a zero-check on the receiver parameter. This violates the best practices for writing secure smart contracts and introduces a potential vulnerability.

Vulnerability Detail

A function that transfers Ether or tokens to an address should check that the address is not zero. A zero address is an invalid address that represents the genesis block of the blockchain. Sending Ether or tokens to a zero address is equivalent to burning them, as they cannot be recovered or accessed by anyone. However, the function BaseStrategyVault.redeemFromNotional(address,address,uint256,uint256,uint256,bytes) does not check that the receiver parameter is not zero. Instead, it transfers Ether or tokens to the receiver address without any validation. This means that if the receiver parameter is zero, the function will burn Ether or tokens instead of sending them to the intended recipient. This is a dangerous and risky behavior, as it could result in loss of funds, unauthorized transfers, or denial of service.

Impact

The impact of this vulnerability depends on how the function BaseStrategyVault.redeemFromNotional(address,address,uint256,uint256,uint256,bytes) is used and what the receiver parameter represents. If the function is called by other functions in the contract or by external users, it could lead to loss of funds, unauthorized transfers, or denial of service. For example, if the receiver parameter is zero, then the function will burn Ether or tokens instead of sending them to the intended recipient. This could result in loss of funds for the sender or the recipient, or unauthorized transfers for the contract. Alternatively, if the receiver parameter is not zero, then the function will send Ether or tokens to the receiver address as expected. However, the function could still be vulnerable to malicious attacks that exploit the lack of validation. For example, an attacker could pass a zero address as the receiver parameter and cause the function to burn Ether or tokens, or pass a malicious address as the receiver parameter and cause the function to send Ether or tokens to an unauthorized address.

Code Snippet

Source Link:- https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L175-L206

function redeemFromNotional(
    address account,
    address receiver,
    uint256 vaultShares,
    uint256 maturity,
    uint256 underlyingToRepayDebt,
    bytes calldata data
) external onlyNotional returns (uint256 transferToReceiver) {
    uint256 borrowedCurrencyAmount = _redeemFromNotional(account, vaultShares, maturity, data);

    uint256 transferToNotional;
    if (account == address(this) || borrowedCurrencyAmount <= underlyingToRepayDebt) {
        // It may be the case that insufficient tokens were redeemed to repay the debt. If this
        // happens the Notional will attempt to recover the shortfall from the account directly.
        // This can happen if an account wants to reduce their leverage by paying off debt but
        // does not want to sell strategy tokens to do so.
        // The other situation would be that the vault is calling redemption to deleverage or
        // settle. In that case all tokens go back to Notional.
        transferToNotional = borrowedCurrencyAmount;
    } else {
        transferToNotional = underlyingToRepayDebt;
        unchecked { transferToReceiver = borrowedCurrencyAmount - underlyingToRepayDebt; }
    }

    if (_UNDERLYING_IS_ETH) {
        // Lacks a zero-check on the receiver parameter
        if (transferToReceiver > 0) payable(receiver).transfer(transferToReceiver);
        if (transferToNotional > 0) payable(address(NOTIONAL)).transfer(transferToNotional);
    } else {
        // Lacks a zero-check on the receiver parameter
        if (transferToReceiver > 0) _UNDERLYING_TOKEN.checkTransfer(receiver, transferToReceiver);
        if (transferToNotional > 0) _UNDERLYING_TOKEN.checkTransfer(address(NOTIONAL), transferToNotional);
    }
}

Tool used

Manual Review

Recommendation

The recommendation to fix this vulnerability is to check that the address is not zero. This can be done by using the require statement or the Address library from the OpenZeppelin library.

function redeemFromNotional(
    address account,
    address receiver,
    uint256 vaultShares,
    uint256 maturity,
    uint256 underlyingToRepayDebt,
    bytes calldata data
) external onlyNotional returns (uint256 transferToReceiver) {
    uint256 borrowedCurrencyAmount = _redeemFromNotional(account, vaultShares, maturity, data);

    uint256 transferToNotional;
    if (account == address(this) || borrowedCurrencyAmount <= underlyingToRepayDebt) {
        // It may be the case that insufficient tokens were redeemed to repay the debt. If this
        // happens the Notional will attempt to recover the shortfall from the account directly.
        // This can happen if an account wants to reduce their leverage by paying off debt but
        // does not want to sell strategy tokens to do so.
        // The other situation would be that the vault is calling redemption to deleverage or
        // settle. In that case all tokens go back to Notional.
        transferToNotional = borrowedCurrencyAmount;
    } else {
        transferToNotional = underlyingToRepayDebt;
        unchecked { transferToReceiver = borrowedCurrencyAmount - underlyingToRepayDebt; }
    }

    if (_UNDERLYING_IS_ETH) {
        // Checks that the receiver address is not zero
@>  require(receiver != address(0), "Zero address");
        if (transferToReceiver > 0) payable(receiver).transfer(transferToReceiver);
        if (transferToNotional > 0) payable(address(NOTIONAL)).transfer(transferToNotional);
    } else {
        // Checks that the receiver address is not zero
@>  require(Address.isNotZero(receiver), "Zero address");
        if (transferToReceiver > 0) _UNDERLYING_TOKEN.checkTransfer(receiver, transferToReceiver);
        if (transferToNotional > 0) _UNDERLYING_TOKEN.checkTransfer(address(NOTIONAL), transferToNotional);
    }
}

sherlock-audit / 2023-10-notional-judging

Im_th3AK - Missing zero address validation for Ether or token transfers. #59