139.md

amaechieth

medium

insufficient validation can lead to loss of funds for user

Summary

In D3Proxy the user essentially swaps tokens using sellTokens. However, this function doesn't validate the case where a user sends msg.value but doesn't specify ETH/Native address. This lack of validation doesn't protect the user from front-end errors that may supply incorrect values or from their own accidental misconfiguration.

This loss of funds may be significant and they should be protected from this risk.

Vulnerability Detail

D3Proxy.sol#L80-L101

function sellTokens(
        address pool,
        address to,
        address fromToken,
        address toToken,
        uint256 fromAmount,
        uint256 minReceiveAmount,
        bytes calldata data,
        uint256 deadLine
    ) public payable judgeExpired(deadLine) returns (uint256 receiveToAmount) {
        if (fromToken == _ETH_ADDRESS_) {
            require(msg.value == fromAmount, "D3PROXY_VALUE_INVALID");
            receiveToAmount = ID3MM(pool).sellToken(to, _WETH_, toToken, fromAmount, minReceiveAmount, data);
        } else if (toToken == _ETH_ADDRESS_) {
            receiveToAmount =
                ID3MM(pool).sellToken(address(this), fromToken, _WETH_, fromAmount, minReceiveAmount, data);
            _withdrawWETH(to, receiveToAmount);
            // multicall withdraw weth to user
        } else {
            receiveToAmount = ID3MM(pool).sellToken(to, fromToken, toToken, fromAmount, minReceiveAmount, data);
        }
    }

This function considers 3 cases:

1. fromToken == ETH_ADDRESS meaning the user wants to deposit ETH/Native in exchange for another token. In this case, they are expected to provide msg.value equal to the number of tokens they are sending fromAmount 2 & 3. fromToken != ETH_ADDRESS meaning the user wants to deposit an ERC20 token for another ERC20 token or ETH/Native

In cases 2&3 the user it is assumed that the user is not sending any msg.value however this is not enforced. As the sellToken function is payable the user is able to send any msg.value which will be lost in these cases.

Impact

Lack of protection for user loss of funds

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/periphery/D3Proxy.sol#L80-L101

Tool used

Manual Review

Recommendation

in case 2&3 the following check should be done if (msg.value != 0) revert()