DefiRound Manual Review Findings

DRD-01M: Improper Token Sanitization

Description:

The withdraw function does not properly sanitize the input token, permitting a user to withdraw any token they wish thus interchanging the assets held by the contract freely at a one-to-one rate or higher depending on the number of decimals.

Example:

Copy
112function withdraw(TokenData calldata tokenInfo, bool asETH) external override {
113    require(currentStage > STAGES.STAGE_1, "WITHDRAWS_NOT_ACCEPTED");
114    require(!_isLastLookComplete(), "WITHDRAWS_EXPIRED");
115
116    TokenData memory data = tokenInfo;
117    address token = data.token;
118    uint256 tokenAmount = data.amount;
119    require(supportedTokens.contains(token), "UNSUPPORTED_TOKEN");
120    require(tokenAmount > 0, "INVALID_AMOUNT");
121    AccountData storage tokenAccountData = accountData[msg.sender];
122    tokenAccountData.currentBalance = tokenAccountData.currentBalance.sub(tokenAmount);
123    // set the data back in the mapping, otherwise updates are not saved
124    accountData[msg.sender] = tokenAccountData;
125
126    // Don't transfer WETH, WETH is converted to ETH and sent to the recipient
127    if (token == WETH && asETH) {
128        IWETH(WETH).withdraw(tokenAmount);
129        msg.sender.sendValue(tokenAmount);            
130    }  else {
131        IERC20(token).safeTransfer(msg.sender, tokenAmount);
132    }
133    
134    emit Withdrawn(msg.sender, tokenInfo, asETH);
135}

Recommendation:

We advise a require check to additionally be imposed whereby the token is ensured to be equal to the tokenAccountData.token, thereby preventing this scenario from unfolding.

Alleviation:

The token argument is now properly validated with a user's accountData.

DRD-02M: Potential for Lock of Ether

Description:

The deposit function does not properly account for the code path whereby a non-zero msg.value has been supplied with a token other than WETH.

Example:

Copy
75// Convert ETH to WETH if ETH is passed in, otherwise treat WETH as a regular ERC20
76if (token == WETH && msg.value > 0) {
77    require(tokenAmount == msg.value, "INVALID_MSG_VALUE"); 
78    IWETH(WETH).deposit{value: tokenAmount}();
79}

Recommendation:

We advise an else branch to be added to the linked if clause that ensures msg.value is equal to 0 to prevent Ether from being locked in the contract.

Alleviation:

The if-else structure now properly ensures that msg.value is equal to zero on the else branch.

DRD-03M: Potentially Locked Ether

Description:

The receive function of the contract does not impose any access control and permits accidentally sent Ether to be permanently locked.

Example:

Copy
109// solhint-disable-next-line no-empty-blocks
110receive() external payable { }

Recommendation:

We advise a require check to be introduced whereby only the WETH address is allowed to sent Ether to the contract for the unwrapping and wrapping mechanisms.

Alleviation:

The msg.sender of the receive function is now properly restricted to be the WETH address.