✅RACE #15 - DEX

RACE #15 Of The Secureum Bootcamp Epoch∞ • Ventral DigitalVentral Digital

RACE #15

RACE #15 result

All 8 questions in this RACE are based on the below contract. This is the same contract you will see for all the 8 questions in this RACE. The question is below the shown contract.

// SPDX-License-Identifier: agpl-3.0
pragma solidity ^0.8.0;

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.0.0/contracts/token/ERC20/IERC20.sol";

contract SimpleDEX {
    uint64 public token_balance;
    uint256 public current_eth;
    IERC20 public token;
    uint8 public reentrancy_lock;
    address owner;
    uint256 public fees;
    uint256 public immutable fees_percentage = 10;

    modifier nonReentrant(){
        // Logic needs to be implemented    
        _; 
    }

    modifier onlyOwner(){
        require(tx.origin == owner, "Only owner permitted");
        _;
    }

    constructor(uint first_balance, address _token, address _owner) payable {
        require(_owner != address(0) , "No zero Address allowed");
        require(msg.value >= 100);
        token = IERC20(_token);
        bool token_success = token.transferFrom(msg.sender, address(this), first_balance);
        require (token_success, "couldn't transfer tokens");
        owner = _owner;
        token_balance = uint64(first_balance);
        current_eth = msg.value;
    }

    function setOwner(address _owner) public onlyOwner {
        owner = _owner;
    }

    function getTokenBalance() public view returns(uint64 _token_balance) {
        _token_balance = token_balance;
    }

    function getCurrentEth() public view returns(uint256 _current_eth) {
        _current_eth = current_eth;
    }

    function getEthPrice() public view returns(uint) {
        return uint256(token_balance) / current_eth;
    }

    function claimFees() public onlyOwner {
        bool token_success =  token.transfer(msg.sender,fees);
        require(token_success, "couldn't transfer tokens");
        token_balance -= fees; 
        fees = 0;
    }

    function buyEth(uint amount) external nonReentrant {
        require(amount >= 10);
        uint ratio = getEthPrice();
        uint fee = (amount / 100) * fees_percentage;
        uint token_amount = ratio * (amount + fee);
        bool token_success = token.transferFrom(msg.sender, address(this), token_amount);
        current_eth -= amount;
        require(token_success, "couldn't transfer tokens");
        (bool success, ) = msg.sender.call{value: amount}("");
        require(success, "Failed to transfer Eth");
        token_balance += uint64(token_amount);
        fees += ratio * fee; 
    }

    fallback() payable external {
        revert();
    }
}


contract SimpleDexProxy {
    function buyEth(address simpleDexAddr, uint amount) external {
        require(amount > 0, "Zero amount not allowed");
        (bool success, ) = (simpleDexAddr).call(abi.encodeWithSignature("buyEth(uint)", amount));
        require (success, "Failed");
    }
}


contract Seller {
    // Sells tokens to the msg.sender in exchange for eth, according to SimpleDex's getEthPrice() 
    function buyToken(SimpleDEX simpleDexAddr, uint token_amount) external payable {
        uint ratio = simpleDexAddr.getEthPrice();
        IERC20 token = simpleDexAddr.token(); 
        uint eth = token_amount / ratio;
        require(token_amount == ratio *eth); //only exact exchange 
        require(eth >= msg.value);
        token.transfer(msg.sender, token_amount);
    }
}

Question 1 ✅

What is/are the correct implementation(s) of the nonReentrant() modifier?

• A.

require (reentrancy_lock == 1); 
reentrancy_lock = 0;
_;
reentrancy_lock = 1;

• B.

require (reentrancy_lock == 0); 
reentrancy_lock = 1;
_;
reentrancy_lock = 0;

• C.

require (reentrancy_lock == 1 ); 
reentrancy_lock = 1;
_;
reentrancy_lock = 0;

• D.

require (reentrancy_lock == 0); 
reentrancy_lock = 2;
_;
reentrancy_lock = 0;

My comment:

These two correct options are the same as:

require (reentrancy_lock == false); // must be unlocked
reentrancy_lock = true; // lock it
_;
reentrancy_lock = false; // unlock it

Question 2 ✅

Who can claim fees using claimFees()?

• A. Only the owner, due to onlyOwner modifier

• B. The owner

• C. Anyone who can trick owner into signing an arbitrary transaction

• D. No one

My comment:

The onlyOwner modifier is vulnerable to phishing attack:

modifier onlyOwner(){
    require(tx.origin == owner, "Only owner permitted");
    _;
}

Question 3 ✅

In buyEth(), we put an unchecked block on current_eth -= amount

• A. Because current_eth is uint

• B. Because the compiler is protecting us from overflows

• C. Only if we add a prior check: require(current_eth > amount);

• D. Only if we add a prior check: require(current_eth >= amount);

Question 4 - This one tricky

In buyEth(), are there any reentrancy concerns assuming the nonReentrant modifier is implemented correctly?

• A. No, because it has the nonReentrant modifier

• B. No, and even without the modifier you can't exploit any issue

• C. Yes, there is a cross-contract reentrancy concern via Seller

• D. None of the above

Comment:

While the nonReentrant modifier prevents re-entering the same contract to exploit an "incomplete state", the same cannot be said for other contracts that might make use of the SimpleDEX's state before the state is completely updated.

Specifically state variables involved in determining the price (token_balance & current_eth) are relevant here: current_eth is updated before the call() to the message sender is made. But token_balance is only updated after.

If the msg.sender is actually a contract, it will have a chance to call another protocol that is relying on the SimpleDEX's reported price to be correct (such as the Seller contract). If the malicious contract calls this victim contract while the state of SimpleDEX is incomplete (ie. cross-contract read-only reentrancy) the victim would make use of this incorrect price data which might give the attacker an advantage. (Not in this case though. There's no advantage to exploiting this in Seller since the attacker would actually have to pay a higher price than without exploiting this issue).

Question 5 ✅

What will happen when calling buyEth() via SimpleDexProxy?

• A. buyEth() will be called and successfully executed

• B. You can’t call a function that way; it must be called directly

• C. buyEth() will be called but ETH won't be transferred

• D. Transaction will be reverted

Comment:

The transaction would be reverted since the SimpleDEX's buyEth() function would attempt transferring the tokens from the msg.sender, which in this case would be a proxy that has no way to give it the appropriate allowance even if the user were to transfer their tokens to the proxy first.

Question 6 ✅

In buyEth():

• A. If amount is less than 100, it will lead to an incorrect calculation of fee -> (amount / 100) * fees_percentage division before multiplication causes precision loss

• B. If token_balance is already at its MAX_UINT256, it will result in overflow and won't revert

• C. If token_amount > MAX_UINT64, it will result in a casting issue -> will be truncated

• D. None of the above

Question 7 ✅

Can getEthPrice() return zero?

• A. Yes, if the owner initializes the contract with more ETH than token_balance -> uint256(token_balance) / current_eth precision loss

• B. Yes, a carefully crafted buyEth() transaction can result in getEthPrice() returning zero ->

• C. Yes, once all the ETH are sold -> will revert because of division by 0

• D. No, there is no issue

Question 8 - This one tricky

Which of the following invariants (written in propositional logic) hold on a correct implementation of the code?

• A. this.balance == current_eth <=> token.balanceOf(this) == token_balance

• B. this.balance >= current_eth && token.balanceOf(this) >= token_balance

• C. this.balance <= token.balanceOf(this) && token.balanceOf(this) <= token_balance

• D. this.balance >= current_eth || token.balanceOf(this) >= token_balance

Comment:

The symbol <=> is called the biconditional operator, meaning that the expressions on either side are logically equivalent. But the actual balance of ether being equal to the balance tracked within the state variable current_eth does not imply that the actual and tracked token balances are equal too. So the biconditional is invalid. But even if it were an AND operator it would not be an invariant that could hold: The invariant would be simple to break by sending unsolicited tokens to the contract (eg. via selfdestruct()).

Option B) includes the fact that the actual token and ether balances may be higher than the tracked balances. In a correct implementation this invariant should always hold.

Option C)'s second part allows the tracked token_balance to be larger than the actual token balance. This should not be the case in a correct implementation.

Option D) seems similar to B) but would allow for there to be either too little ether to match the tracked balance or too little tokens to match the tracked token balance. So it doesn't sound like a good invariant to test for since you'd want both things to hold true and not just one of them. But that wasn't the question - would it hold true in a correct implementation? Yes.