Fuel Labs Predeposit Contract Security Review Report

The deposit() and depositWithPermit() functions will put an incorrect amount of tokens into the balances mapping if the transfer amount changes during the token calls (like in fee-on-transfer tokens).

This will lead to other users losing funds on the withdraw() call.

contracts/PreDeposits/PreDeposits.sol:L65

    IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);

contracts/PreDeposits/PreDeposits.sol:L90

    IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);

Use the difference between balances before and after the transfers.

The function accepts and emits the depositParam argument in the Deposit event but doesn't actually update it in the storage deposits mapping.

You can see the update in the twin deposit() function:

contracts/PreDeposits/PreDeposits.sol:L62-L63

            _tokenDeposit.depositParam = depositParam;
            deposits[_msgSender()][token] = _tokenDeposit;

function depositWithPermit(
	address token,
	uint240 amount,
	uint16 depositParam,
	uint256 deadline,
	uint8 v,
	bytes32 r,
	bytes32 s
) external whenNotPaused {
	deposits[_msgSender()][token].balance += amount;
 
	ERC20Permit(token).permit(
		_msgSender(),
		address(this),
		amount,
		deadline,
		v,
		r,
		s
	);
 
	IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);
	emit Deposit(_msgSender(), token, amount, depositParam);
}

Add the update to the function.

The PreDeposits.sol::depositWithPermit() function currently utilizes an inner call to the permit() function of the openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol. However, this flow exposes depositWithPermit() to a griefing attack, where an attacker can forcibly block the victim's transaction.

The attack proceeds as follows: the attacker front-runs the victim's transaction, extracts the parameters from the mempool, and places a transaction that directly calls ERC20Permit(token).permit() with the victim's params. Consequently, the victim's transaction reverts since the signature has already been used for permit() in the attacker's transaction.

    function depositWithPermit(
        address token,
        uint240 amount,
        uint16 depositParam,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
        deposits[_msgSender()][token].balance += amount;
 
        ERC20Permit(token).permit(
            _msgSender(),
            address(this),
            amount,
            deadline,
            v,
            r,
            s
        );
 
        IERC20(token).safeTransferFrom(_msgSender(), address(this), amount);
        emit Deposit(_msgSender(), token, amount, depositParam);
    }

Consider adding try/cath, or if block, so if the permit() is already called, just check the allowance of msg.sender and skip the call to permit().

The current implementation of the PreDeposits.sol contract restricts withdrawals to the sender's address. This limitation can be restrictive for users who might want to withdraw their funds to a different address. Allowing withdrawals to other addresses can offer flexibility and convenience to users, facilitating smoother fund management and better user experience.

    function withdraw(address token, uint240 amount) external whenNotPaused {
        // Underflow checks already in effect with new solidity versions
        deposits[_msgSender()][token].balance =
            deposits[_msgSender()][token].balance -
            amount;
 
        if (token == address(0)) {
            (bool success, ) = _msgSender().call{ value: amount }("");
            if (!success) {
                revert RecipientRevert();
            }
        } else {
            IERC20(token).safeTransfer(_msgSender(), amount);
        }
 
        emit Withdraw(_msgSender(), token, amount);
    }

Add address recipient to the withdraw() function.

The migrate() function is typically used for migration management. However, in this case, it is not implemented and will always revert.

    function migrate(
        address /*token*/,
        address /*migrationFacilitator*/,
        bytes calldata /*facilitatorData*/
    ) external view whenNotPaused {
        revert("UNIMPLEMENTED");
    }

Consider implementing the migrate() function.