Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Asynchronous burn reentrancy bug #17

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix: Asynchronous burn reentrancy bug #17

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26300f4
fix: Asynchronous burn reentrancy bug
silasdavis Nov 19, 2024
54b4f20
fix: make _burnCallback error-free to ensure locked token amount is
silasdavis Nov 19, 2024
19cfafd
feat: add _cancelBurn to cancel an in-flight burn request
silasdavis Nov 19, 2024
93c598e
fix: ordering of locking
silasdavis Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 64 additions & 15 deletions contracts/ConfidentialERC20/ConfidentialERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -50,7 +54,9 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada
struct BurnRq {
address account;
uint64 amount;
bool exists;
}

mapping(uint256 => BurnRq) public burnRqs;
/**
* @dev Returns the name of the token.
Expand Down Expand Up @@ -109,8 +115,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]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not like this essential, universal check being the responsibility of the caller, so it is moved to _transfer and the logic is centralised in _hasSufficientBalance which also considers the _lockedBalances

_transfer(owner, to, value, isTransferable);
_transfer(owner, to, value, TFHE.asEbool(true));
return true;
}

Expand Down Expand Up @@ -142,6 +147,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;
Expand Down Expand Up @@ -178,6 +184,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`.
*
Expand All @@ -195,7 +212,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));
Expand Down Expand Up @@ -232,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be exposed to the original sender/unwrapper if they wanted to cancel in-flight. We ought to be able to rely on the callback being called though, so we shouldn't need this just for cleaning up in-flight burns.

The EOA cna use the BurnRequested event to keep track of this requestId so it's possible to give them cancellation

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.
Expand All @@ -244,7 +285,9 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada
if (account == address(0)) {
revert ERC20InvalidReceiver(address(0));
}
ebool enoughBalance = TFHE.le(amount, _balances[account]);
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);
Expand All @@ -256,22 +299,31 @@ 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);
}

function _burnCallback(uint256 requestID, bool decryptedInput) public virtual onlyGateway {
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;
if (!decryptedInput) {
revert("Decryption failed");
Copy link
Contributor Author

@silasdavis silasdavis Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bit of a misleading error message

// Unlock the burn amount unconditionally
_lockedBalances[account] = _lockedBalances[account] - amount;
delete burnRqs[requestID];
if (!hasEnoughBalance) {
emit InsufficientBalanceToBurn(account, amount);
return;
}
_totalSupply = _totalSupply - 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.
Expand Down Expand Up @@ -334,14 +386,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]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_transfer now handles the balance check

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`.
*
Expand Down
Loading