Forgotten Runes Warrior Guild contest - z3s's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L608-L619 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L164

Vulnerability details

Impact

To withdraw ETH, it uses send(), this transaction will fail inevitably when:

1. The withdrawer smart contract does not implement a payable function.
2. Withdrawer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The withdrawer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

/**
 * @notice Withdraw funds to the vault
 * @param _amount uint256 the amount to withdraw
 */
function withdraw(uint256 _amount) public onlyOwner {
    require(address(vault) != address(0), 'no vault');
    require(payable(vault).send(_amount));
}

/**
 * @notice Withdraw all funds to the vault
 */
function withdrawAll() public payable onlyOwner {
    require(address(vault) != address(0), 'no vault');
    require(payable(vault).send(address(this).balance));
}

So if vault has one of above conditions. sending funds will fail. because vault address can be updated, I don't send this as High issue.

Recommended Mitigation Steps

/**
 * @notice Withdraw funds to the vault
 * @param _amount uint256 the amount to withdraw
 */
function withdraw(uint256 _amount) public onlyOwner {
    require(address(vault) != address(0), 'no vault');
    (bool success, ) = vault.call{value: _amount}("");
    require(success);
}

/**
 * @notice Withdraw all funds to the vault
 */
function withdrawAll() public payable onlyOwner {
    require(address(vault) != address(0), 'no vault');
    (bool success, ) = vault.call{value: address(this).balance}("");
    require(success);
}

And here same problem:

ForgottenRunesWarriorsGuild.sol
  164,9:         require(payable(msg.sender).send(address(this).balance));

If owner has one of above conditions, too. owner can't withdraw wrongly sent eths.

Recommended Mitigation Steps

ForgottenRunesWarriorsGuild.sol:

function withdrawAll() public payable onlyOwner {
    (bool success, ) = msg.sender.call{value: address(this).balance}("");
    require(success);
}

Non Critical

[N01] Delete payable from withdrawAll():

withdrawAll() do not use msg.value and just withdraw eth so it doesn't need to be payable.

Recommended Mitigation Steps:

  - function withdrawAll() public payable onlyOwner {
  + function withdrawAll() public onlyOwner {

ForgottenRunesWarriorsMinter.sol:616
  - function withdrawAll() public payable onlyOwner {
  + function withdrawAll() public onlyOwner {

[N02] Check .transfer() return's value:

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures.

ForgottenRunesWarriorsGuild.sol
  175,9:         token.transfer(msg.sender, amount);

ForgottenRunesWarriorsMinter.sol
  629,9:         token.transfer(msg.sender, amount);

ForgottenRunesWarriorsMinter.sol
  402,13:             IERC20(weth).transfer(to, amount);

Recommended Mitigation Steps

Consider using safeTransfer or check transfer's return with require().

[N03] type(uint256).max is more readable than hex version of the number:

ForgottenRunesWarriorsMinter.sol
  18,9:         0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
  23,9:         0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
  27,9:         0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
  31,9:         0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
  35,9:         0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

Recommended Mitigation Steps:

Use type(uint256).max instead of 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff.

Gas Optimizations

Use Custom Errors to save Gas:

Custom errors from Solidity 0.8.4 are cheaper than require messages. https://blog.soliditylang.org/2021/04/21/custom-errors/