EthPool Manual Review Findings

EPL-01M: Inapplicacy of Checks-Effects-Interactions Pattern

Description:

The withdraw function does not properly apply the Checks-Effects-Interactions pattern, leading to a possible state whereby the underlying Ether will have been transferred to the caller but they will still be in possession of the token units of the pool.

Example:

Copy
99if (asEth) {
100    weth.withdraw(amount);
101    msg.sender.sendValue(amount);
102} else {
103    IERC20(weth).safeTransfer(msg.sender, amount);
104}
105
106_burn(msg.sender, amount);

Recommendation:

We advise the _burn statement to be relocated above the if-else branch that transfers Ether outwards to ensure that no illogical state can be reached.

Alleviation:

The _burn statement was set to precede the actual withdrawal of the stake thereby alleviating this exhibit.

EPL-02M: Incorrect allowance Arguments

Description:

The arguments for the allowance call are incorrect.

Example:

Copy
144function approveManager(address account, uint256 amount) external override onlyOwner {
145    uint256 currentAllowance = IERC20(weth).allowance(account, account);
146    if (currentAllowance < amount) {
147        IERC20(weth).safeIncreaseAllowance(account, type(uint256).max.sub(currentAllowance));
148    }
149}

Recommendation:

We advise the address(this) argument to be passed in the allowance call as the first argument to properly enforce the logic of the function.

Alleviation:

The statements were properly updated to use the correct arguments.

EPL-03M: Potential for Lock of Ether

Description:

The _deposit function does not properly handle a positive value for both amount and msgValue which should be prevented as it currently leads to loss of funds.

Example:

Copy
167function _deposit(address fromAccount, address toAccount, uint256 amount, uint256 msgValue) internal {
168    require(amount > 0, "INVALID_AMOUNT");
169    require(toAccount != address(0), "INVALID_ADDRESS");
170
171    if (msgValue > 0) {
172        require(msgValue == amount, "AMT_VALUE_MISMATCH");
173        weth.deposit{value: amount}();
174    } else {            
175        IERC20(weth).safeTransferFrom(fromAccount, address(this), amount);
176    }
177    _mint(toAccount, amount);
178}

Recommendation:

We advise a require check to be introduced in the else branch of the function that ensures msg.value is equal to 0.

Alleviation:

The Tokemak team has stated that the contract is meant to receive ETH in ways that cannot be protected and that they have opted to retain the contract as-is.