code-423n4 / 2022-09-y2k-finance-findings

3 stars 1 forks source link

SOLMATE'S SAFETRANSFERLIB DO NOT HAVE CONTRACT EXISTENCE CHECKS #177

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L85-L101 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L110-L128 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L90-L107 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L132-L139 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L213-L223

Vulnerability details

Impact

Solmate's safetransferlib does not check for contract existence https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

The functions safeTransferFrom() and safeTransfer() from SafeTransferLib.sol does not check if a token contract actually contains code. Thus, if the provided token address has no code, these functions will not revert as low-level calls to non-contracts always return success.

Below are the instances where token transfers are made without checking for code existence:

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L85-L101 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L110-L128 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L90-L107 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L132-L139 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L213-L223

// SemiFungibleVault.sol L85-L101
    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    ) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, id, shares, EMPTY);

        emit Deposit(msg.sender, receiver, id, assets, shares);

        afterDeposit(id, assets, shares);
    }

Proof of Concept

asset.safeTransferFrom does not revert the transaction when the asset is zero address

Bob could call deposit() in SemiFungibleVault.sol contract with asset as zero address

The contract mint shares for Bob

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check for contract existence

HickupHH3 commented 2 years ago

True, but: