Blur Exchange contest - neko_nyaa's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L13

Vulnerability details

Impact

The Pool contract uses the UUPSUpgradeable pattern, which requires an initialize function to be used. However, there is no initialize() in the Pool contract, nor does there exists any call of __Ownable_init(). The result is that there will be no owner of the Pool contract.

This impact is considered through a direct Discord message from the warden with the sponsors

We have additional plans in the future that will make the pool more functional, that’s why it is upgradeable.

Hence, with the loss of ownership, the following effects can happen:

• The team will not be able to implement extra functions to the pool, as expected.
• Suppose the team recognizes this issue, then the team no choice other than to deploy a new contract instance. This significantly impacts user experience and gas costs due to having to move funds to a new contract, furthermore this also impacts the team reputation.

Proof of concept

The following test proves that it is not possible for an admin to upgrade the Pool contract.

/// @audit To run this test, add the import line to any existing test file
/// then, copy paste the following test and run

// add this import
import {deployPool} from '../scripts/deploy';

// copy paste this test
describe('Wrong upgrade test', async () => {
  it('fail upgrade', async () => {
    const pool1 = await deployPool();
    const pool2 = await deployPool(); // or any contract address, not necessarily a pool
      
    // for verification
    //console.log("OWNER ADDR:", await pool1.owner()); // 0x000.....
    //console.log("ADMIN ADDR:", admin.address);       // 0xf39....

    await expect(
      pool1.connect(admin).upgradeTo(pool2.address)
    ).to.be.revertedWith('Ownable: caller is not the owner');
  });
});

Tools used

VSCode

Recommended mitigation steps

There are two steps needed to mitigate this issue: Add the missing initializings in the contract, and fixing the deployment script to call the initialize function.

• Contract fix: Add the following functions into the contract

constructor() {
    _disableInitializers();
}

function initialize() external initializer {
    __Ownable_init();
}

It is not strictly needed to add a constructor, however it is considered good practice to run _disableInitializers() to prevent anyone from initializing the implementation contract, to better protect the users.

• Deployment fix: In the file scripts/deploy.ts, there is a function deployPool(). Change it to as following:

export async function deployPool(): Promise<Contract> {
  const poolImpl = await deploy('Pool', [], {}, 'PoolImpl');
  const initializeInterface = new hre.ethers.utils.Interface([
    'function initialize()',
  ]);
  const initialize = initializeInterface.encodeFunctionData('initialize', []);
  const poolProxy = await deploy(
    'ERC1967Proxy',
    [poolImpl.address, initialize],
    {},
    'Pool',
  );
  const pool = new hre.ethers.Contract(
    poolProxy.address,
    poolImpl.interface,
    poolProxy.signer,
  );
  return pool;
}

This will correctly deploy a pool instance, as well as appropriately calling the initialize function.

The test shown in the PoC will fail (the upgradeTo() call will be successful) upon applying the recommended mitigations.

Low risk findings:

1. It is possible for Pool.totalSupply() to return wrong value
2. ExecutionDelegate.transferERC20 does not check for contract existence
3. Misleading/wrong event definition in Pool.sol
4. Pool.sol: Missing storage gap in upgradeable contract pattern

Non-risk findings:

1. Missing reason string in require
2. Two-step ownership transfer is recommended

[L-01] It is possible for Pool.totalSupply() to return wrong value

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L83

function totalSupply() public view returns (uint256) {
    return address(this).balance;
}

The function Pool.totalSupply() simply returns the ETH balance of the Pool contract. It may seem like the way to "wrap" ETH into the contract are calling deposit() or through receive(), which in turn just calls deposit().

However, one can use selfdestruct on an ETH-holding contract, which will forcefully send ETH to the recipient contract without calling any functions. This will cause Pool.totalSupply() to return the wrong value, and may lead to accounting issues.

Recommended mitigation steps: Consider tracking the total supply separately when a user deposits and withdraws.

[L-02] ExecutionDelegate.transferERC20 does not check for contract existence

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/ExecutionDelegate.sol#L127-L130

bytes memory returndata = token.functionCall(data);
if (returndata.length > 0) {
    require(abi.decode(returndata, (bool)), "ERC20 transfer failed");
}

If the supposedly-ERC20 address does not contain a contract, then returndata.length will be 0, and the check will not be performed at all, causing the transfer to silently succeed without actually transferring any data.

It is worth noting that the token has to be admin-approved first, however it is still good practice to minimize human error (e.g. if admin makes a mistake).

Recommended mitigation steps: Consider using Openzeppelin's SafeERC20, or implementing a contract check. The contract check can be done during the admin-approval step instead of the transferring step, to save user gas cost.

[L-03] Misleading/wrong event definition in Pool.sol

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L37 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L49 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L76

Let us note the definition of the Transfer event:

event Transfer(address indexed from, address indexed to, uint256 amount);

Implying that a transfer from "amount" is made by "from" to "to".

When transferring from one user to another:

emit Transfer(from, to, amount);

When depositing, the event emitted is:

emit Transfer(msg.sender, address(0), msg.value);

When withdrawing:

emit Transfer(address(0), msg.sender, amount);

The problem is that, the definition for depositing and withdrawing very misleading for contract readers, especially for those who are familiar and frequently work with ERC20 standard tokens. According to the EIP-20:

• Minting tokens should emit transfer event from 0 to recipient.
• Burning tokens should emit transfer event from sender to 0.

(Note that the standard does not strictly enforce this, but does make this a guideline)

The pool contract is implemented much like an ERC-20, except with pre-approval, and without being able to approve other addresses. It is not strictly required to conform to ERC-20 standard since this is technically not a token.

However considering the similarities the Pool contract has to ERC-20 and its intended usage, I believe this is a problem for any external developers looking to understand the protocol, as well as performing any operations related to events in general.

Recommended mitigation steps: Swap the events for depositing and withdrawing.

[L-04] Pool.sol: Missing storage gap in upgradeable contract pattern

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L13

It is recommended to add a storage gap in an ungradeable contract, to avoid collision when upgrading.

Consider adding this line, following the pattern of existing upgradeable contracts in the repo (lib/EIP712.sol and lib/ReentrancyGuarded.sol).

uint256[50] private __gap;

[N-01] Missing reason string in require

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L45

require(_balances[msg.sender] >= amount);

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L48

(bool success,) = payable(msg.sender).call{value: amount}("");
require(success); // @audit there is no reason string
emit Transfer(address(0), msg.sender, amount);

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L71-L72

require(_balances[from] >= amount);
require(to != address(0));

It is good practice to state the error for easier investigation when an error occurs. In this case the transfer may revert without a reason string.

Also consider using custom errors, as stated in [GAS-5] of C4udit.

[N-02] Two-step ownership transfer is recommended

Several contracts are marked as Ownable, however ownership can be lost due to human error if it is transferred to a non-EOA.

It is recommended to implement a two-step transferring method, where the new owner has to claim ownership.