From 26300f4778a5e79ff0501a75cb0b5dcb123df99f Mon Sep 17 00:00:00 2001 From: Silas Davis Date: Tue, 19 Nov 2024 18:03:16 +0700 Subject: [PATCH 1/4] fix: Asynchronous burn reentrancy bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original report: >>>Hello, I think there may be an issue with the ConfidentialERC20Wrapper (https://github.com/Inco-fhevm/confidential-erc20-framework/blob/main/contracts/ConfidentialERC20Wrapper.sol).It seems possible to transfer funds while the Gateway is pending decryption (https://github.com/Inco-fhevm/confidential-erc20-framework/blob/main/contracts/ConfidentialERC20/ConfidentialERC20.sol#L247). Since the underflow is checked only when the unwrap function is called, it would be possible to steal all the ERC20 wrapped in the contract.For instance, let's say my encrypted balance is equal to 1. I call the unwrap() function with amount=1. Before the Gateway decrypts (and calls the callback function _burnCallback), I transfer my 1 to another address. Once the gateway calls the fallback, it would make my balance equal to type(uint64).max . Then, I can check the (decrypted) totalSupply and call another unwrap() passing amount = totalSupply . Once the gateway calls the callback (_burnCallback), it would transfer all the tokens to me Signed-off-by: Silas Davis --- .../ConfidentialERC20/ConfidentialERC20.sol | 48 ++++++++++++++----- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/contracts/ConfidentialERC20/ConfidentialERC20.sol b/contracts/ConfidentialERC20/ConfidentialERC20.sol index d654a11..b8f0c5e 100644 --- a/contracts/ConfidentialERC20/ConfidentialERC20.sol +++ b/contracts/ConfidentialERC20/ConfidentialERC20.sol @@ -30,6 +30,10 @@ import "fhevm/gateway/GatewayCaller.sol"; abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metadata, IERC20Errors, GatewayCaller { mapping(address account => euint64) public _balances; + // To safely handle burn requests we must ensure we lock up an amount of token that is in-flight so that the + // we can guarantee there will be sufficient balance to be burnt at the point that _burnCallback runs. + mapping(address account => uint64) public _lockedBalances; + mapping(address account => mapping(address spender => euint64)) internal _allowances; uint64 public _totalSupply; @@ -51,6 +55,7 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada address account; uint64 amount; } + mapping(uint256 => BurnRq) public burnRqs; /** * @dev Returns the name of the token. @@ -109,8 +114,7 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada function transfer(address to, euint64 value) public virtual returns (bool) { require(TFHE.isSenderAllowed(value)); address owner = _msgSender(); - ebool isTransferable = TFHE.le(value, _balances[msg.sender]); - _transfer(owner, to, value, isTransferable); + _transfer(owner, to, value, TFHE.asEbool(true)); return true; } @@ -142,6 +146,7 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada _approve(owner, spender, value); return true; } + function approve(address spender, einput encryptedAmount, bytes calldata inputProof) public virtual returns (bool) { approve(spender, TFHE.asEuint64(encryptedAmount, inputProof)); return true; @@ -178,6 +183,17 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada return true; } + /** + * @dev Checks the available balance of an account exceeds amount + */ + function _hasSufficientBalance(address owner, euint64 amount) internal virtual returns (ebool) { + return TFHE.le(TFHE.add(amount, _lockedBalances[owner]), _balances[owner]); + } + + function _hasSufficientBalance(address owner, uint64 amount) internal virtual returns (ebool) { + return _hasSufficientBalance(owner, TFHE.asEuint64(amount)); + } + /** * @dev Moves a `value` amount of tokens from `from` to `to`. * @@ -195,7 +211,12 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada if (to == address(0)) { revert ERC20InvalidReceiver(address(0)); } - euint64 transferValue = TFHE.select(isTransferable, value, TFHE.asEuint64(0)); + // Enforce sufficient balance constraint universally + euint64 transferValue = TFHE.select( + TFHE.and(_hasSufficientBalance(from, value), isTransferable), + value, + TFHE.asEuint64(0) + ); euint64 newBalanceTo = TFHE.add(_balances[to], transferValue); _balances[to] = newBalanceTo; TFHE.allow(newBalanceTo, address(this)); @@ -244,7 +265,9 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada if (account == address(0)) { revert ERC20InvalidReceiver(address(0)); } - ebool enoughBalance = TFHE.le(amount, _balances[account]); + // Unconditionally lock the burn amount + _lockedBalances[account] = _lockedBalances[account] + amount; + ebool enoughBalance = _hasSufficientBalance(account, amount); TFHE.allow(enoughBalance, address(this)); uint256[] memory cts = new uint256[](1); cts[0] = Gateway.toUint256(enoughBalance); @@ -260,14 +283,18 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada burnRqs[requestID] = BurnRq(account, amount); } - function _burnCallback(uint256 requestID, bool decryptedInput) public virtual onlyGateway { + error InsufficientBalanceToBurn(address account, uint64 burnAmount); + + function _burnCallback(uint256 requestID, bool hasEnoughBalance) public virtual onlyGateway { BurnRq memory burnRequest = burnRqs[requestID]; address account = burnRequest.account; uint64 amount = burnRequest.amount; - if (!decryptedInput) { - revert("Decryption failed"); + if (!hasEnoughBalance) { + revert InsufficientBalanceToBurn(account, amount); } _totalSupply = _totalSupply - amount; + // Unlock the burn amount + _lockedBalances[account] = _lockedBalances[account] - amount; _balances[account] = TFHE.sub(_balances[account], amount); TFHE.allow(_balances[account], address(this)); TFHE.allow(_balances[account], account); @@ -334,14 +361,11 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada */ function _decreaseAllowance(address owner, address spender, euint64 amount) internal virtual returns (ebool) { euint64 currentAllowance = _allowances[owner][spender]; - - ebool allowedTransfer = TFHE.le(amount, currentAllowance); - - ebool canTransfer = TFHE.le(amount, _balances[owner]); - ebool isTransferable = TFHE.and(canTransfer, allowedTransfer); + ebool isTransferable = TFHE.le(amount, currentAllowance); _approve(owner, spender, TFHE.select(isTransferable, TFHE.sub(currentAllowance, amount), currentAllowance)); return isTransferable; } + /** * @dev Increases `owner` s allowance for `spender` based on spent `value`. * From 54b4f2022807e267737104657bcba17e8e19e5c0 Mon Sep 17 00:00:00 2001 From: Silas Davis Date: Tue, 19 Nov 2024 12:58:56 +0100 Subject: [PATCH 2/4] fix: make _burnCallback error-free to ensure locked token amount is always freed Signed-off-by: Silas Davis --- contracts/ConfidentialERC20/ConfidentialERC20.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/ConfidentialERC20/ConfidentialERC20.sol b/contracts/ConfidentialERC20/ConfidentialERC20.sol index b8f0c5e..c94b999 100644 --- a/contracts/ConfidentialERC20/ConfidentialERC20.sol +++ b/contracts/ConfidentialERC20/ConfidentialERC20.sol @@ -283,22 +283,23 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada burnRqs[requestID] = BurnRq(account, amount); } - error InsufficientBalanceToBurn(address account, uint64 burnAmount); + event InsufficientBalanceToBurn(address account, uint64 burnAmount); function _burnCallback(uint256 requestID, bool hasEnoughBalance) public virtual onlyGateway { BurnRq memory burnRequest = burnRqs[requestID]; address account = burnRequest.account; uint64 amount = burnRequest.amount; + // Unlock the burn amount unconditionally + _lockedBalances[account] = _lockedBalances[account] - amount; + delete burnRqs[requestID]; if (!hasEnoughBalance) { - revert InsufficientBalanceToBurn(account, amount); + emit InsufficientBalanceToBurn(account, amount); + return; } _totalSupply = _totalSupply - amount; - // Unlock the burn amount - _lockedBalances[account] = _lockedBalances[account] - amount; _balances[account] = TFHE.sub(_balances[account], amount); TFHE.allow(_balances[account], address(this)); TFHE.allow(_balances[account], account); - delete burnRqs[requestID]; } /** * @dev Sets `value` as the allowance of `spender` over the `owner` s tokens. From 19cfafd0b0430dd563ef0223dfee06e8b4e436ef Mon Sep 17 00:00:00 2001 From: Silas Davis Date: Tue, 19 Nov 2024 13:06:38 +0100 Subject: [PATCH 3/4] feat: add _cancelBurn to cancel an in-flight burn request Signed-off-by: Silas Davis --- .../ConfidentialERC20/ConfidentialERC20.sol | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/contracts/ConfidentialERC20/ConfidentialERC20.sol b/contracts/ConfidentialERC20/ConfidentialERC20.sol index c94b999..9708073 100644 --- a/contracts/ConfidentialERC20/ConfidentialERC20.sol +++ b/contracts/ConfidentialERC20/ConfidentialERC20.sol @@ -54,6 +54,7 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada struct BurnRq { address account; uint64 amount; + bool exists; } mapping(uint256 => BurnRq) public burnRqs; @@ -253,6 +254,25 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada _totalSupply += value; } + event BurnRequested(uint256 requestId, address account, uint64 amount); + + event BurnRequestCancelled(uint256 requestID); + + error BurnRequestDoesNotExist(uint256 requestId); + + function _cancelBurn(uint256 requestId) internal virtual { + BurnRq memory burnRequest = burnRqs[requestId]; + if (!burnRequest.exists) { + revert BurnRequestDoesNotExist(requestId); + } + address account = burnRequest.account; + uint64 amount = burnRequest.amount; + // Unlock the burn amount unconditionally + _lockedBalances[account] = _lockedBalances[account] - amount; + delete burnRqs[requestId]; + emit BurnRequestCancelled(requestId); + } + /** * @dev Destroys a `value` amount of tokens from `account`, lowering the total supply. * Relies on the `_update` mechanism. @@ -279,14 +299,18 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada block.timestamp + 100, false ); - - burnRqs[requestID] = BurnRq(account, amount); + burnRqs[requestID] = BurnRq(account, amount, true); + emit BurnRequested(requestID, account, amount); } event InsufficientBalanceToBurn(address account, uint64 burnAmount); function _burnCallback(uint256 requestID, bool hasEnoughBalance) public virtual onlyGateway { BurnRq memory burnRequest = burnRqs[requestID]; + if (!burnRequest.exists) { + revert BurnRequestDoesNotExist(requestID); + return; + } address account = burnRequest.account; uint64 amount = burnRequest.amount; // Unlock the burn amount unconditionally From 93c598eee917237c62fe21f9f00552716021f84b Mon Sep 17 00:00:00 2001 From: Silas Davis Date: Wed, 20 Nov 2024 16:54:13 +0100 Subject: [PATCH 4/4] fix: ordering of locking Signed-off-by: Silas Davis --- contracts/ConfidentialERC20/ConfidentialERC20.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/ConfidentialERC20/ConfidentialERC20.sol b/contracts/ConfidentialERC20/ConfidentialERC20.sol index 9708073..400345d 100644 --- a/contracts/ConfidentialERC20/ConfidentialERC20.sol +++ b/contracts/ConfidentialERC20/ConfidentialERC20.sol @@ -285,9 +285,9 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada if (account == address(0)) { revert ERC20InvalidReceiver(address(0)); } - // Unconditionally lock the burn amount - _lockedBalances[account] = _lockedBalances[account] + amount; ebool enoughBalance = _hasSufficientBalance(account, amount); + // Unconditionally lock the burn amount after calculating sufficient balance + _lockedBalances[account] = _lockedBalances[account] + amount; TFHE.allow(enoughBalance, address(this)); uint256[] memory cts = new uint256[](1); cts[0] = Gateway.toUint256(enoughBalance);