Malt Finance contest - ScopeLift's results

Handle

ScopeLift

Vulnerability details

Impact

TBD how bad the impact is

The docs mention that the notSameBlock modifier (and associated _notSameBlock() method) is used to guard against reentrancy. However, users can still call a method twice in a single transaction by using transferring assets/positions between two accounts they control, and re-entering with the other account

Proof of Concept

N/A

Tools Used

Recommended Mitigation Steps

Change notSameBlock to a standard reentrancy guard such as this one from Solmate

abstract contract ReentrancyGuard {
    uint256 private reentrancyStatus = 1;

    modifier nonReentrant() {
        require(reentrancyStatus == 1, "REENTRANCY");
        reentrancyStatus = 2;
        _;
        reentrancyStatus = 1;
    }
}

Handle

ScopeLift

Vulnerability details

Impact

The internal _withdraw method does not follow the checks-effects-interactions pattern. A malicious token, or one that implemented transfer hooks, could re-enter the public calling function (such as withdraw()) before proper internal accounting was completed. Because the earned function looks up the _userWithdrawn mapping, which is not yet updated when the transfer occurs, it would be possible for a malicious contract to re-enter _withdraw repeatedly and drain the pool.

Proof of Concept

N/A

Tools Used

N/A

Recommended Mitigation Steps

The internal accounting should be done before the transfer occurs:

function _withdraw(address account, uint256 amountReward, address to) internal {
    _userWithdrawn[account] += amountReward;
    _globalWithdrawn += amountReward;f

   rewardToken.safeTransfer(to, amountReward);

    emit Withdraw(account, amountReward, to);
  }

Handle

ScopeLift

Vulnerability details

Impact

On lines 85 and 101, ETH is transferred using a .call to an address provided as an input, but there is no verification that the call call succeeded. This can result in a call to emergencyWithdrawGAS or partialWithdrawGAS appearing successful but in reality it failed. This can happen when the provided destination address is a contract that cannot receive ETH, or if the amount provided is larger than the contract's balance

Proof of Concept

Enter the following in remix, deploy the Receiver contract, and send 1 ETH when deploying the Permissions contract. Call emergencyWithdrawGAS with the receiver address and you'll see it reverts. This would not be caught in the current code

pragma solidity ^0.8.0;

contract Receivier{}

contract Permissions {
    constructor() payable {}

    function emergencyWithdrawGAS(address payable destination) external {
        (bool ok, ) = destination.call{value: address(this).balance}('');
        require(ok, "call failed");
    }
}

Tools Used

Remix

Recommended Mitigation Steps

In emergencyWithdrawGAS:

- destination.call{value: address(this).balance}('');
+ (bool ok, ) = destination.call{value: address(this).balance}('');
+ require(ok, "call failed");

And similar for partialWithdrawGAS