✅RACE #12 - ERC20 Permit

RACE #12 of the Secureum Bootcamp Epoch∞ • Ventral DigitalVentral Digital

RACE #12

RACE #12 result

Note: 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: GPL-3.0

pragma solidity 0.8.17;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";

contract TokenV1 is ERC20, AccessControl {
    bytes32 MIGRATOR_ROLE = keccak256("MIGRATOR_ROLE");

    constructor() ERC20("Token", "TKN") {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
    }

    // Spec wasn't clear about what 'admin functions' need to be capable of.
    // Well, this should do the trick.
    fallback() external {
        if (hasRole(MIGRATOR_ROLE, msg.sender)) {
            (bool success, bytes memory data) = msg.sender.delegatecall(msg.data);
            require(success, "MIGRATION CALL FAILED");
            assembly {
                return(add(data, 32), mload(data))
            }
        }
    }
}

interface IEERC20 is IERC20, IERC20Permit {}

contract Vault {
    address public UNDERLYING;
    mapping(address => uint256) public balances;

    constructor(address token)  {
        UNDERLYING = token;
    }

    function deposit(uint256 amount) external {
        IEERC20(UNDERLYING).transferFrom(msg.sender, address(this), amount);
        balances[msg.sender] += amount;
    }

    function depositWithPermit(address target, uint256 amount, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) external {
        IEERC20(UNDERLYING).permit(target, address(this), amount, deadline, v, r, s);
        IEERC20(UNDERLYING).transferFrom(target, address(this), amount);
        balances[to] += amount;
    }

    function withdraw(uint256 amount) external {
        IEERC20(UNDERLYING).transfer(msg.sender, amount);
        balances[msg.sender] -= amount;
    }

    function sweep(address token) external {
        require(UNDERLYING != token, "can't sweep underlying");
        IEERC20(token).transfer(msg.sender, IEERC20(token).balanceOf(address(this)));
    }
}

/* ... some time later ... */

// Adding permit() while maintaining old token balances.
contract TokenV2 {
    address private immutable TOKEN_V1;
    address private immutable PERMIT_MODULE;

    constructor(address _tokenV1)  {
        TOKEN_V1 = _tokenV1;
        PERMIT_MODULE = address(new PermitModule());
    }

    // Abusing migrations as proxy.
    fallback() external {
        (
            bool success,
            bytes memory data
        ) = (address(this) != TOKEN_V1)
          ? TOKEN_V1.call(abi.encodePacked(hex"00000000", msg.data, msg.sender))
          : PERMIT_MODULE.delegatecall(msg.data[4:]);
        require(success, "FORWARDING CALL FAILED");
        assembly {
            return(add(data, 32), mload(data))
        }
    }
}

contract PermitModule is TokenV1, ERC20Permit {
    constructor() TokenV1() ERC20Permit("Token") {}
    function _msgSender() internal view virtual override returns (address) {
        if (address(this).code.length == 0) return super._msgSender(); // normal context during construction
        return address(uint160(bytes20(msg.data[msg.data.length-20:msg.data.length])));
    }
}

Question 1 ✅

Sensible gas optimization(s) would be

• A. Making MIGRATOR_ROLE state variable constant -> Can be constant

• B. Making UNDERLYING state variable constant -> Can be immutable but not constant

• C. Making MIGRATOR_ROLE state variable immutable -> Can be immutable, but being constant makes more sense

• D. Making UNDERLYING state variable immutable -> Can be immutable

Comment:

While MIGRATOR_ROLE can be made both constant or immutable, using constant makes the most sense since the value is known during compile time.

UNDERLYING on the other hand can only be immutable since the value is passed in during the contract's construction.

Question 2 ✅

What would a caller with MIGRATOR_ROLE permission be capable of?

• A. Manipulating TokenV1's storage -> This is possbile via msg.sender.delegatecall(msg.data)

• B. Deleting TokenV1's stored bytecode -> Call selfdestruct() in msg.sender.delegatecall(msg.data)

• C. Changing TokenV1's stored bytecode to something different -> Only when TokenV1 was deployed via CREATE2

• D. With the current code it's not possible for anyone to have MIGRATOR_ROLE permission -> grantRole() is inherited from OpenZeppelin AccessControl

Comment:

A contract that was given the MIGRATOR_ROLE will be capable of triggering TokenV1's fallback function which will delegate-call them back. During this delegate-call, the contract will be capable of manipulating storage as well as self-destructing TokenV1.

Replacing the bytecode would only be possible if TokenV1 was deployed via CREATE2, allowing for a redeployment at the same address with a different bytecode.

The public grantRole() function is inherited from AccessControl and allows callers with DEFAULT_ADMIN_ROLE to grant the MIGRATOR_ROLE to any address.

Question 3 ✅

Vault initialized with TokenV1 as underlying

• A. Can be drained by re-entering during withdrawal -> The target token contract is set in the constructor, it must be malicious or implements a callback

• B. Can be drained during withdrawal due to an integer underflow -> Solidity version is 0.8.17, not possible

• C. Allows stealing approved tokens due to a phantom (i.e. missing) function -> Attacker calls Vault.depositWithPermit(), IEERC20(UNDERLYING).permit() will be called, and then msg.sender.delegatecall(msg.data) will be triggered. Since Vault inherits permit() from IERC20Permit, the delegatecall would succeed. Back to Vault.depositWithPermit(), IEERC20(UNDERLYING).transferFrom(target, address(this), amount) will be called. Later attacker can call Vault.withdraw() to take out the money.

• D. None of the above

Comment:

A Vault initialized with TokenV1 offers no opportunity to re-enter although the Checks-Effects-Interactions pattern is not being followed. For this to be exploitable the underlying token itself must either be malicious or implement something akin to receive-hooks like ERC-777.

The fact that a Solidity version >0.8.0 is used without any unchecked blocks should prevent any integer over or underflows from happening.

The depositWithPermit() function is relying on the call to TokenV1's permit() method to revert if it's not implemented or the provided signature is invalid. However, TokenV1's fallback() function will make any calls to permit() succeed, making permit() a "phantom function". With any calls succeeding an attacker would be able to make deposits using the allowances of other users without the need for a valid signature.

Question 4 - Does not make sense

If Vault were to use safeTransferFrom instead of transferFrom then

• A. It would be able to safely support tokens that don't revert on error

• B. It would ensure that tokens are only sent to contracts that support handling them

• C. It would introduce a re-entrancy vulnerability due to receive hooks

• D. None of the above

Comment:

The way Vault is currently implemented, its deployer needs to be careful to not use a token as underlying that returns success booleans instead of reverting on error. Using the SafeERC20 library would add the safeTranserFrom() function and allow both kinds of token implementations to be used.

Answers B and C talk about the "safe" methods of NFT standards such as ERC721. These would check whether a receiving contract declares supporting them and would also offer an opportunity for re-entrancy via the onERC721Received() hook.

My comment:

This question is confusing. What type of safeTransferFrom() is used here? If we are talking about ERC721, then A,B,C are all correct. ERC20 does not have safeTransferFrom() anyway.

Question 5 ✅

Who would need the MIGRATOR_ROLE for TokenV2 to function as intended?

• A. The deployer of the TokenV2 contract -> The deployer needs DEFAULT_ADMIN_ROLE instead

• B. The TokenV1 contract

• C. The TokenV2 contract -> TokenV2 needs MIGRATOR_ROLE to trigger the fallback function in TokenV1

• D. The PermitModule contract

Comment:

The deployer of TokenV2 would need DEFAULT_ADMIN_ROLE for granting it the MIGRATOR_ROLE.

TokenV2 is the only contract that requires the role since it needs to (ab)use TokenV1's fallback function trigger a delegate-call to the PermitModule's contract.

Question 6 ✅

With TokenV2 deployed, a Vault initialized with TokenV1 as underlying

• A. Is no longer vulnerable in the depositWithPermit() function -> As long as TokenV1 is used as UNDERLYING, this vulnerability exists

• B. Becomes more vulnerable due to a Double-Entry-Point -> Attacker can call sweep(TokenV2), then this call will be forwarded to TokenV1 and those token will be transferred to msg.sender (attacker)

• C. Stops functioning because TokenV1 has been replaced -> Nonsense

• D. None of the above

Comment:

A Vault initialized with TokenV1 will always be vulnerable in the depositWithPermit() function. A new Vault would need to start using TokenV2 for the depositWithPermit() function to no longer be vulnerable to the permit()-phantom.

TokenV2 now acts as a Double-Entry-Point for TokenV1 (and vice versa). This can be exploited via the sweep() function which allows rescuing any stuck tokens as long as they are not the underlying token. Sweeping TokenV2 on a Vault using TokenV1 would effectively drain the Vault.

Question 7 ✅

Vault initialized with TokenV2 as underlying

• A. Can be drained by re-entering during withdrawal -> Same as Question 3

• B. Can be drained during withdrawal due to a integer underflow -> Same as Question 3

• C. Is not vulnerable in the depositWithPermit() function -> IEERC20(UNDERLYING).permit() will call PermitModule, so this is different from Question 3

• D. Is vulnerable due to a Double-Entry-Point -> Same as Question 6

Comment:

Answers A and B haven't changed from Question 3 with the introduction of TokenV2.

In TokenV2 calling the permit() function will no longer call the fallback but its actual implementation from ERC20Permit. Therefore a Vault with TokenV2 will no longer be vulnerable to the permit()-phantom.

TokenV1 acts as a Double-Entry-Point for TokenV2 (and vice versa). This can be exploited via the sweep() function which allows rescuing any stuck tokens as long as they are not the underlying token. Sweeping TokenV1 on a Vault using TokenV2 would effectively drain the Vault.

Question 8 - This one tricky

The PermitModule contract

• A. Acts as a proxy -> No, TokenV2 is the proxy

• B. Acts as an implementation -> Yes, TokenV2 forwards calls to PermitModule

• C. Allows anyone to manipulate TokenV2's balances

• D. Can be self-destructed by anyone

Comment:

TokenV2 will forward delegate-calls made from TokenV2 to the PermitModule contract. Effectively TokenV2 abuses the migration logic in TokenV1 turning it into a proxy while PermitModule acts like the implementation.

The way that Context's _msgSender() function has been overridden, it allows any caller to arbitrarily set what is expected to be the msg.sender. By crafting a call with TokenV1's address appended they will gain the DEFAULT_ADMIN_ROLE. With that, they can grant themselves the MIGRATOR_ROLE and make PermitModule delegate-call back. Since all token balances are stored at TokenV1's address this will not allow for a manipulation of any real balances. But it would allow delegate-calling to a contract that'll execute a self-destruct.