pwning_dev - unchecked transfers in the `_mint` and `_burn` functions

[sherlock-admin2]

pwning_dev

Medium

unchecked transfers in the _mint and _burn functions

pwning_dev

Summary

Vulnerability Detail

In the _mint function, the transferFrom method of the nst token is called without checking its return value:

function _mint(uint256 assets, uint256 shares, address receiver) internal {
    require(receiver != address(0) && receiver != address(this), "SNst/invalid-address");

    nst.transferFrom(msg.sender, address(this), assets); // Unchecked transfer

    unchecked {
        balanceOf[receiver] = balanceOf[receiver] + shares;
        totalSupply = totalSupply + shares;
    }

    emit Deposit(msg.sender, receiver, assets, shares);
    emit Transfer(address(0), receiver, shares);
}

In the _burn function, the transfer method of the nst token is called without checking its return value:

function _burn(uint256 assets, uint256 shares, address receiver, address owner) internal {
    uint256 balance = balanceOf[owner];
    require(balance >= shares, "SNst/insufficient-balance");

    if (owner != msg.sender) {
        uint256 allowed = allowance[owner][msg.sender];
        if (allowed != type(uint256).max) {
            require(allowed >= shares, "SNst/insufficient-allowance");

            unchecked {
                allowance[owner][msg.sender] = allowed - shares;
            }
        }
    }

    unchecked {
        balanceOf[owner] = balance - shares;
        totalSupply      = totalSupply - shares;
    }

    nst.transfer(receiver, assets); // Unchecked transfer

    emit Transfer(owner, address(0), shares);
    emit Withdraw(msg.sender, receiver, owner, assets, shares);
}

Impact

Scenario 1: Transfer Fails During Minting

If the transferFrom call fails during the _mint operation:

• User Impact: The user who initiated the minting will not have their tokens transferred, but the contract will still issue shares. This means the user gets shares without actually providing the underlying tokens.
• Contract Impact: The balanceOf and totalSupply for the shares will increase inaccurately. The contract's perceived holdings of the underlying token will not match the actual token balance.

Scenario 2: Transfer Fails During Burning

If the transfer call fails during the _burn operation:

• User Impact: The user who initiated the burning will have their shares burned, but the underlying tokens will not be transferred back to them. They effectively lose their shares without receiving the equivalent value in tokens.
• Contract Impact: The balanceOf and totalSupply for the shares will decrease, but the actual tokens will still be in the contract. This can lead to a build-up of tokens in the contract that do not correspond to any issued shares.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/sdai/src/SNst.sol#L284C5-L324C1

Tool used

Manual Review

Recommendation

_mint Function with Checked Transfer

function _mint(uint256 assets, uint256 shares, address receiver) internal {
    require(receiver != address(0) && receiver != address(this), "SNst/invalid-address");

    bool success = nst.transferFrom(msg.sender, address(this), assets);
    require(success, "SNst/transferFrom-failed"); // Check the transfer

    unchecked {
        balanceOf[receiver] = balanceOf[receiver] + shares;
        totalSupply = totalSupply + shares;
    }

    emit Deposit(msg.sender, receiver, assets, shares);
    emit Transfer(address(0), receiver, shares);
}

_burn Function with Checked Transfer

function _burn(uint256 assets, uint256 shares, address receiver, address owner) internal {
    uint256 balance = balanceOf[owner];
    require(balance >= shares, "SNst/insufficient-balance");

    if (owner != msg.sender) {
        uint256 allowed = allowance[owner][msg.sender];
        if (allowed != type(uint256).max) {
            require(allowed >= shares, "SNst/insufficient-allowance");

            unchecked {
                allowance[owner][msg.sender] = allowed - shares;
            }
        }
    }

    unchecked {
        balanceOf[owner] = balance - shares;
        totalSupply      = totalSupply - shares;
    }

    bool success = nst.transfer(receiver, assets);
    require(success, "SNst/transfer-failed"); // Check the transfer

    emit Transfer(owner, address(0), shares);
    emit Withdraw(msg.sender, receiver, owner, assets, shares);
}

[telome]

Checking the return value of the transfer is unnecessary as nst reverts if the transfer fails.