# RACE #12 - ERC20 Permit {% embed url="" %} RACE #12 {% endembed %}

RACE #12 result

{% hint style="info" %} *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.* {% endhint %} ```solidity // 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 :white\_check\_mark: Sensible gas optimization(s) would be * [x] 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 * [x] 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 :white\_check\_mark: What would a caller with MIGRATOR\_ROLE permission be capable of? * [x] A. Manipulating TokenV1's storage -> This is possbile via `msg.sender.delegatecall(msg.data)` * [x] 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 :white\_check\_mark: 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 * [x] 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 * [x] 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 :white\_check\_mark: 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 * [x] 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 :white\_check\_mark: 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 * [x] 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 :white\_check\_mark: 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 * [x] C. Is not vulnerable in the depositWithPermit() function -> `IEERC20(UNDERLYING).permit()` will call PermitModule, so this is different from Question 3 * [x] 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 * [x] B. Acts as an implementation -> Yes, TokenV2 forwards calls to PermitModule * [ ] C. Allows anyone to manipulate TokenV2's balances * [x] 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.**