Egis-Security / CTF_Challenge

Repository containing CTF challenges from nmirchev8, deth and bOgO.
14 stars 8 forks source link

deth_ctf - Factory contract does NOT return vault address #8

Closed 0xSpacePirate closed 2 months ago

0xSpacePirate commented 2 months ago

Description of the Bug:

In Factory::deployVault the idea of the function is to deploy a newly created Vault, once the deployed the Vault returns its address on the blockchain, however this address is never returned:

vaultAddress = address(new Vault{salt: salt}(msg.sender));

Hence, a user paid gas to execute the Factory::deployVault but did not get the address of their newly deployed contract and they will NOT know which contract they can use to deposit/withdraw and unlock.

In addition, the lastDeployed variable is created but never updated.

Impact: Impact: High Likelihood: High

User cannot access their vault contract since they will not know the address.

Solution:

Add the following lines in the Factory::deployVault to prevent this issue:

-    function deployVault() external {
+    function deployVault() external returns (address) {
        address vaultAddress = computeAddress();

        if (vaultAddress.codehash != bytes32(0)) {
            revert AlreadyDeployed();
        } 

        bytes32 salt = bytes32(uint256(uint160(msg.sender)));
        vaultAddress = address(new Vault{salt: salt}(msg.sender));
+      lastDeployed = vaultAddress;  // low severity issue fix
+      return vaultAddress;  // high severity issue fix
    }
0xdeth commented 2 months ago

Duplicate of #4

0xSpacePirate commented 2 months ago

https://github.com/Egis-Security/CTF_Challenge/issues/4 is focused on: lastDeployed = vaultAddress:

Which is not that big of an issue since if a user calls the function immediately after another user then the track of lastDeployed is lost.

My issue focuses on the missing return vaultAddress;

For instance, in a single transaction:

  1. User A calls Factory::deployVault.
  2. User B calls Factory::deployVault.
  3. User C calls Factory::deployVault.

lastDeployed = User C vault address. User A & User B addresses are permamently lost.

Hence, the missing return vaultAddress is a way bigger issue than the one in https://github.com/Egis-Security/CTF_Challenge/issues/4.