Skip to content

Commit

Permalink
Limit the number of twin transfers (#800)
Browse files Browse the repository at this point in the history 
* poc

* Limit max reserved gas

* handle tests under coverage

---------

Co-authored-by: Mischa <mischa@mmt.me.uk>
  • Loading branch information
@zajck @mischat
zajck and mischat authored Sep 13, 2023
1 parent 2b195f8 commit aab19d9
Show file tree
Hide file tree
Showing 4 changed files with 389 additions and 129 deletions.
1 change: 1 addition & 0 deletions contracts/interfaces/events/IBosonTwinEvents.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ interface IBosonTwinEvents {
uint256 amount,
address executedBy
);
event TwinTransferSkipped(uint256 indexed exchangeId, uint256 twinCount, address indexed executedBy);
}
3 changes: 3 additions & 0 deletions contracts/interfaces/handlers/IBosonExchangeHandler.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ interface IBosonExchangeHandler is IBosonExchangeEvents, IBosonFundsLibEvents, I
* @notice Redeems a voucher.
*
* Emits a VoucherRedeemed event if successful.
* Emits TwinTransferred if twin transfer was successfull
* Emits TwinTransferFailed if twin transfer failed
* Emits TwinTransferSkipped if twin transfer was skipped when the number of twins is too high
*
* Reverts if
* - The exchanges region of protocol is paused
Expand Down
279 changes: 150 additions & 129 deletions contracts/protocol/facets/ExchangeHandlerFacet.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,9 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
* @notice Redeems a voucher.
*
* Emits a VoucherRedeemed event if successful.
* Emits TwinTransferred if twin transfer was successfull
* Emits TwinTransferFailed if twin transfer failed
* Emits TwinTransferSkipped if twin transfer was skipped when the number of twins is too high
*
* Reverts if
* - The exchanges region of protocol is paused
Expand Down Expand Up @@ -833,6 +836,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
* Emits ERC20 Transfer, ERC721 Transfer, or ERC1155 TransferSingle events in call stack if successful.
* Emits TwinTransferred if twin transfer was successfull
* Emits TwinTransferFailed if twin transfer failed
* Emits TwinTransferSkipped if twin transfer was skipped when the number of twins is too high
*
* If one of the twin transfers fails, the function will continue to transfer the remaining twins and
* automatically raises a dispute for the exchange.
Expand Down Expand Up @@ -869,150 +873,167 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
address sender = msgSender();
uint256 twinCount = twinIds.length;

// Fetch twin: up to 20,000 gas
// Handle individual outcome: up to 120,000 gas
// Handle overall outcome: up to 200,000 gas
// SINGLE_TWIN_RESERVED_GAS = 160000
// MINIMAL_RESIDUAL_GAS = 230000
// Next line would overflow if twinCount > (type(uint256).max - MINIMAL_RESIDUAL_GAS)/SINGLE_TWIN_RESERVED_GAS
// Oveflow happens for twinCount ~ 9.6x10^71, which is impossible to achieve
// Oveflow happens for twinCount ~ 7.2x10^71, which is impossible to achieve
uint256 reservedGas = (twinCount - 1) * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS;

// Visit the twins
for (uint256 i = 0; i < twinCount; ) {
// Get the twin
(, Twin storage twinS) = fetchTwin(twinIds[i]);

// Use twin struct instead of individual variables to avoid stack too deep error
// Don't copy the whole twin to memory immediately, only the fields that are needed
Twin memory twinM;
twinM.tokenId = twinS.tokenId;
twinM.amount = twinS.amount;

bool success;
{
twinM.tokenType = twinS.tokenType;

// Shouldn't decrement supply if twin supply is unlimited
twinM.supplyAvailable = twinS.supplyAvailable;
if (twinM.supplyAvailable != type(uint256).max) {
// Decrement by 1 if token type is NonFungible otherwise decrement amount (i.e, tokenType is MultiToken or FungibleToken)
twinM.supplyAvailable = twinM.tokenType == TokenType.NonFungibleToken
? twinM.supplyAvailable - 1
: twinM.supplyAvailable - twinM.amount;

twinS.supplyAvailable = twinM.supplyAvailable;
}
// If number of twins is too high, skip the transfer and mark the transfer as failed.
// Reserved gas is higher than the actual gas needed for succesful twin redeem.
// There is enough buffer that even if the reserved gas is above gas limit, the redeem will still succeed.
// This check was added to prevent the DoS attack where the attacker would create a bundle with a huge number of twins.
// For a normal operations this still allows for a bundle with more than 180 twins to be redeemed, which should be enough for practical purposes.
if (reservedGas > block.gaslimit) {
transferFailed = true;

emit TwinTransferSkipped(_exchange.id, twinCount, sender);
} else {
// Visit the twins
for (uint256 i = 0; i < twinCount; ) {
// Get the twin
(, Twin storage twinS) = fetchTwin(twinIds[i]);

// Use twin struct instead of individual variables to avoid stack too deep error
// Don't copy the whole twin to memory immediately, only the fields that are needed
Twin memory twinM;
twinM.tokenId = twinS.tokenId;
twinM.amount = twinS.amount;

bool success;
{
twinM.tokenType = twinS.tokenType;

// Transfer the token from the seller's assistant to the buyer
bytes memory data; // Calldata to transfer the twin

if (twinM.tokenType == TokenType.FungibleToken) {
// ERC-20 style transfer
data = abi.encodeCall(IERC20.transferFrom, (assistant, sender, twinM.amount));
} else if (twinM.tokenType == TokenType.NonFungibleToken) {
// Token transfer order is ascending to avoid overflow when twin supply is unlimited
if (twinM.supplyAvailable == type(uint256).max) {
twinS.tokenId++;
} else {
// Token transfer order is descending
twinM.tokenId += twinM.supplyAvailable;
// Shouldn't decrement supply if twin supply is unlimited
twinM.supplyAvailable = twinS.supplyAvailable;
if (twinM.supplyAvailable != type(uint256).max) {
// Decrement by 1 if token type is NonFungible otherwise decrement amount (i.e, tokenType is MultiToken or FungibleToken)
twinM.supplyAvailable = twinM.tokenType == TokenType.NonFungibleToken
? twinM.supplyAvailable - 1
: twinM.supplyAvailable - twinM.amount;

twinS.supplyAvailable = twinM.supplyAvailable;
}
// ERC-721 style transfer
data = abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,bytes)",
assistant,
sender,
twinM.tokenId,
""
);
} else if (twinM.tokenType == TokenType.MultiToken) {
// ERC-1155 style transfer
data = abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,uint256,bytes)",
assistant,
sender,
twinM.tokenId,
twinM.amount,
""
);
}
// Make call only if there is enough gas and code at address exists.
// If not, skip the call and mark the transfer as failed
twinM.tokenAddress = twinS.tokenAddress;
uint256 gasLeft = gasleft();
if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) {
address to = twinM.tokenAddress;

// Handle the return value with assembly to avoid return bomb attack
bytes memory result;
assembly {
success := call(
sub(gasLeft, reservedGas), // gasleft()-reservedGas
to, // twin contract
0, // ether value
add(data, 0x20), // invocation calldata
mload(data), // calldata length
add(result, 0x20), // store return data at result
0x20 // store at most 32 bytes
)

let returndataSize := returndatasize()

switch gt(returndataSize, 0x20)
case 0 {
// Adjust result length in case it's shorter than 32 bytes
mstore(result, returndataSize)
}
case 1 {
// If return data is longer than 32 bytes, consider transfer unsuccesful
success := false

// Transfer the token from the seller's assistant to the buyer
bytes memory data; // Calldata to transfer the twin

if (twinM.tokenType == TokenType.FungibleToken) {
// ERC-20 style transfer
data = abi.encodeCall(IERC20.transferFrom, (assistant, sender, twinM.amount));
} else if (twinM.tokenType == TokenType.NonFungibleToken) {
// Token transfer order is ascending to avoid overflow when twin supply is unlimited
if (twinM.supplyAvailable == type(uint256).max) {
twinS.tokenId++;
} else {
// Token transfer order is descending
twinM.tokenId += twinM.supplyAvailable;
}
// ERC-721 style transfer
data = abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,bytes)",
assistant,
sender,
twinM.tokenId,
""
);
} else if (twinM.tokenType == TokenType.MultiToken) {
// ERC-1155 style transfer
data = abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,uint256,bytes)",
assistant,
sender,
twinM.tokenId,
twinM.amount,
""
);
}
// Make call only if there is enough gas and code at address exists.
// If not, skip the call and mark the transfer as failed
twinM.tokenAddress = twinS.tokenAddress;
uint256 gasLeft = gasleft();
if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) {
address to = twinM.tokenAddress;

// Handle the return value with assembly to avoid return bomb attack
bytes memory result;
assembly {
success := call(
sub(gasLeft, reservedGas), // gasleft()-reservedGas
to, // twin contract
0, // ether value
add(data, 0x20), // invocation calldata
mload(data), // calldata length
add(result, 0x20), // store return data at result
0x20 // store at most 32 bytes
)

let returndataSize := returndatasize()

switch gt(returndataSize, 0x20)
case 0 {
// Adjust result length in case it's shorter than 32 bytes
mstore(result, returndataSize)
}
case 1 {
// If return data is longer than 32 bytes, consider transfer unsuccesful
success := false
}
}

// Check if result is empty or if result is a boolean and is true
success =
success &&
(result.length == 0 || (result.length == 32 && abi.decode(result, (uint256)) == 1));
// Check if result is empty or if result is a boolean and is true
success =
success &&
(result.length == 0 || (result.length == 32 && abi.decode(result, (uint256)) == 1));
}
}
}

twinM.id = twinS.id;

// If token transfer failed
if (!success) {
transferFailed = true;
twinM.id = twinS.id;

emit TwinTransferFailed(
twinM.id,
twinM.tokenAddress,
_exchange.id,
twinM.tokenId,
twinM.amount,
sender
);
} else {
ProtocolLib.ProtocolLookups storage lookups = protocolLookups();
uint256 exchangeId = _exchange.id;
// If token transfer failed
if (!success) {
transferFailed = true;

{
// Store twin receipt on twinReceiptsByExchange
TwinReceipt storage twinReceipt = lookups.twinReceiptsByExchange[exchangeId].push();
twinReceipt.twinId = twinM.id;
twinReceipt.tokenAddress = twinM.tokenAddress;
twinReceipt.tokenId = twinM.tokenId;
twinReceipt.amount = twinM.amount;
twinReceipt.tokenType = twinM.tokenType;
}
if (twinM.tokenType == TokenType.NonFungibleToken) {
updateNFTRanges(lookups, twinM, sellerId);
emit TwinTransferFailed(
twinM.id,
twinM.tokenAddress,
_exchange.id,
twinM.tokenId,
twinM.amount,
sender
);
} else {
ProtocolLib.ProtocolLookups storage lookups = protocolLookups();
uint256 exchangeId = _exchange.id;

{
// Store twin receipt on twinReceiptsByExchange
TwinReceipt storage twinReceipt = lookups.twinReceiptsByExchange[exchangeId].push();
twinReceipt.twinId = twinM.id;
twinReceipt.tokenAddress = twinM.tokenAddress;
twinReceipt.tokenId = twinM.tokenId;
twinReceipt.amount = twinM.amount;
twinReceipt.tokenType = twinM.tokenType;
}
if (twinM.tokenType == TokenType.NonFungibleToken) {
updateNFTRanges(lookups, twinM, sellerId);
}
emit TwinTransferred(
twinM.id,
twinM.tokenAddress,
exchangeId,
twinM.tokenId,
twinM.amount,
sender
);
}
emit TwinTransferred(twinM.id, twinM.tokenAddress, exchangeId, twinM.tokenId, twinM.amount, sender);
}

// Reduce minimum gas required for succesful execution
reservedGas -= SINGLE_TWIN_RESERVED_GAS;
// Reduce minimum gas required for succesful execution
reservedGas -= SINGLE_TWIN_RESERVED_GAS;

unchecked {
i++;
unchecked {
i++;
}
}
}
}
Expand Down
Loading

0 comments on commit aab19d9

Please sign in to comment.