Skip to content

Commit

Permalink
[NO-2648] [L-2 Jan Audit] Check return value of transferFrom (#503)
Browse files Browse the repository at this point in the history 
* check transferFrom return value, test

* docgen and snapshot

* rename to isTransferSuccessful

Co-authored-by: amiecorso <amie@nori.com>
  • Loading branch information
@amiecorso
amiecorso and amiecorso committed Jan 25, 2023
1 parent 96981df commit 6a74176
Show file tree
Hide file tree
Showing 7 changed files with 214 additions and 83 deletions.
124 changes: 63 additions & 61 deletions .gas-snapshot
View file
Expand Up @@ -40,57 +40,58 @@ LockedNORITest:testReentryTokensReceived() (gas: 1267234)
LockedNORITest:testReentryTokensToSend() (gas: 1268801)
LockedNORITest:testTokensReceivedReverts() (gas: 69048)
LockedNORILib_availableAmount:test() (gas: 12371)
MarketSupplierSelectionNotUsingUpSuppliersLastRemoval:test() (gas: 888843)
MarketSupplierSelectionNotUsingUpSuppliersLastRemoval:test() (gas: 888848)
Market_ALLOWLIST_ROLE:test() (gas: 12799)
Market__addActiveRemoval:test() (gas: 183344)
Market__addActiveRemoval:test__lis2VintagesFor1SupplierFor2SubIdentifiers() (gas: 242879)
Market__addActiveRemoval:test__list1VintageFor1Supplier() (gas: 188331)
Market__addActiveRemoval:test__list1VintageFor1Supplier() (gas: 188309)
Market__addActiveRemoval:test__list1VintageFor2Suppliers() (gas: 360670)
Market__addActiveRemoval:test__list2VintagesFor1SupplierFor1SubIdentifier() (gas: 262754)
Market__isAuthorizedWithdrawal_false:test_returnsFalseWhenAllConditionsAreFalse() (gas: 7452)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderEqualsOwner() (gas: 443)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderHasDefaultAdminRole() (gas: 96533)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderHasDefaultAdminRole() (gas: 96555)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderIsApprovedForAll() (gas: 9011)
Market__multicall_empty_bytes_reverts:test() (gas: 19318)
Market__multicall_initialize_reverts:test() (gas: 34531)
Market__multicall_empty_bytes_reverts:test() (gas: 19362)
Market__multicall_initialize_reverts:test() (gas: 34619)
Market__setPriceMultiple:test() (gas: 28904)
Market__setPurchasingToken:test() (gas: 29742)
Market__validatePrioritySupply:test_supplyAfterPurchaseIsLessThanPriorityRestrictedThreshold() (gas: 2499)
Market__validatePrioritySupply:test_supplyAfterPurchaseIsZero() (gas: 2522)
Market__validatePrioritySupply_buyerIsAllowlistedAndAmountExceedsPriorityRestrictedThreshold:test() (gas: 4848)
Market__validatePrioritySupply_reverts_LowSupplyAllowlistRequired:test() (gas: 7692)
Market__validatePrioritySupply:test_supplyAfterPurchaseIsLessThanPriorityRestrictedThreshold() (gas: 4625)
Market__validatePrioritySupply:test_supplyAfterPurchaseIsZero() (gas: 4692)
Market__validatePrioritySupply_buyerIsAllowlistedAndAmountExceedsPriorityRestrictedThreshold:test() (gas: 6975)
Market__validatePrioritySupply_reverts_LowSupplyAllowlistRequired:test() (gas: 7710)
Market__validateSupply:test() (gas: 292)
Market__validateSupply:test_reverts_OutOfSupply() (gas: 3150)
Market_getActiveSuppliers:test_1_supplier() (gas: 609800)
Market_getActiveSuppliers:test_3_suppliers() (gas: 1261472)
Market__validateSupply:test_reverts_OutOfSupply() (gas: 3194)
Market_getActiveSuppliers:test_1_supplier() (gas: 609757)
Market_getActiveSuppliers:test_3_suppliers() (gas: 1261343)
Market_getActiveSuppliers:test_no_suppliers() (gas: 20925)
Market_getPriceMultiple:test() (gas: 14962)
Market_getRemovalIdsForSupplier:test_1_removal() (gas: 610115)
Market_getRemovalIdsForSupplier:test_3_removals() (gas: 993016)
Market_getRemovalIdsForSupplier:test_3_removals_different_vintages() (gas: 1039125)
Market_getPriceMultiple:test() (gas: 14873)
Market_getRemovalIdsForSupplier:test_1_removal() (gas: 610072)
Market_getRemovalIdsForSupplier:test_3_removals() (gas: 992973)
Market_getRemovalIdsForSupplier:test_3_removals_different_vintages() (gas: 1039082)
Market_getRemovalIdsForSupplier:test_no_removals() (gas: 26022)
Market_onERC1155BatchReceived:test() (gas: 208124)
Market_onERC1155BatchReceived_reverts_SenderNotRemovalContract:test() (gas: 353218)
Market_onERC1155BatchReceived:test() (gas: 208168)
Market_onERC1155BatchReceived_reverts_SenderNotRemovalContract:test() (gas: 353262)
Market_onERC1155Received:test() (gas: 206036)
Market_onERC1155Received_reverts_SenderNotRemovalContract:test() (gas: 158734)
Market_purchasingTokenAddress:test() (gas: 17169)
Market_setNoriFeePercentage_revertsInvalidPercentage:test() (gas: 20276)
Market_setPriorityRestrictedThreshold:test() (gas: 157404)
Market_setPriorityRestrictedThreshold:test_zeroAvailable() (gas: 152379)
Market_setPurchasingTokenAndPriceMultiple:test() (gas: 34535)
Market_setPurchasingTokenAndPriceMultiple_revertsIfNotAdmin:test() (gas: 50809)
Market_supplierSelectionUsingUpSuppliersLastRemoval:test() (gas: 885552)
Market_swapWithoutFee_emits_and_skips_transfer_when_transferring_wrong_erc20_to_rNori:test() (gas: 402462)
Market_swap_emits_and_skips_transfer_when_transferring_wrong_erc20_to_rNori:test() (gas: 487268)
Market_swap_emits_event_and_skips_mint_when_minting_rNori_to_nonERC1155Receiver:test() (gas: 542360)
Market_withdraw_1x3_center:test() (gas: 340313)
Market_withdraw_2x1_back:test() (gas: 344973)
Market_withdraw_2x1_front:test() (gas: 333330)
Market_withdraw_2x1_front_relist:test() (gas: 381456)
Market_withdraw_as_DEFAULT_ADMIN_ROLE:test() (gas: 276023)
Market_withdraw_as_operator:test() (gas: 285190)
Market_withdraw_as_supplier:test() (gas: 274164)
Market_withdraw_reverts:test() (gas: 138665)
Market_setPriorityRestrictedThreshold:test() (gas: 157403)
Market_setPriorityRestrictedThreshold:test_zeroAvailable() (gas: 152378)
Market_setPurchasingTokenAndPriceMultiple:test() (gas: 34557)
Market_setPurchasingTokenAndPriceMultiple_revertsIfNotAdmin:test() (gas: 50831)
Market_supplierSelectionUsingUpSuppliersLastRemoval:test() (gas: 885556)
Market_swapWithoutFee_emits_and_skips_transfer_when_transferring_wrong_erc20_to_rNori:test() (gas: 402675)
Market_swap_emits_and_skips_transfer_when_transferring_wrong_erc20_to_rNori:test() (gas: 487302)
Market_swap_emits_event_and_skips_mint_when_minting_rNori_to_nonERC1155Receiver:test() (gas: 542385)
Market_swap_revertsWhenUnsafeERC20TransferFails:test() (gas: 157740)
Market_withdraw_1x3_center:test() (gas: 340401)
Market_withdraw_2x1_back:test() (gas: 345061)
Market_withdraw_2x1_front:test() (gas: 333418)
Market_withdraw_2x1_front_relist:test() (gas: 381544)
Market_withdraw_as_DEFAULT_ADMIN_ROLE:test() (gas: 276111)
Market_withdraw_as_operator:test() (gas: 285278)
Market_withdraw_as_supplier:test() (gas: 274252)
Market_withdraw_reverts:test() (gas: 138753)
NORI_name:test() (gas: 17205)
NORI_permit:test() (gas: 92606)
Removal__beforeTokenTransfer:test() (gas: 17504)
Expand All @@ -105,30 +106,30 @@ Removal_addBalance:test() (gas: 59774)
Removal_addBalance_reverts_RemovalNotYetMinted:test() (gas: 31115)
Removal_batchGetHoldbackPercentages_multipleIds:test() (gas: 11098)
Removal_batchGetHoldbackPercentages_singleId:test() (gas: 10346)
Removal_consign_revertsForSoldRemovals:test() (gas: 1065040)
Removal_getMarketBalance:test() (gas: 1075064)
Removal_getOwnedTokenIds:test_multiple_tokens_with_transfer() (gas: 1076217)
Removal_consign_revertsForSoldRemovals:test() (gas: 1064845)
Removal_getMarketBalance:test() (gas: 1074872)
Removal_getOwnedTokenIds:test_multiple_tokens_with_transfer() (gas: 1076130)
Removal_getOwnedTokenIds:test_no_tokens() (gas: 18683)
Removal_getProjectId:test() (gas: 19307)
Removal_grantRole:test_reverts_when_paused() (gas: 26272)
Removal_migrate:test() (gas: 979867)
Removal_migrate_gasLimit:test() (gas: 15250893)
Removal_migrate_revertsIfRemovalBalanceSumDifferentFromCertificateAmount:test() (gas: 994492)
Removal_mintBatch:test() (gas: 347995)
Removal_mintBatch_list:test() (gas: 557502)
Removal_mintBatch_list_sequential:test() (gas: 748384)
Removal_mintBatch_multiple:test_16() (gas: 3079132)
Removal_mintBatch_multiple:test_2() (gas: 725563)
Removal_mintBatch_multiple:test_32() (gas: 5770033)
Removal_mintBatch_multiple:test_4() (gas: 1061748)
Removal_mintBatch_multiple:test_8() (gas: 1734123)
Removal_mintBatch:test() (gas: 347908)
Removal_mintBatch_list:test() (gas: 557459)
Removal_mintBatch_list_sequential:test() (gas: 748298)
Removal_mintBatch_multiple:test_16() (gas: 3079089)
Removal_mintBatch_multiple:test_2() (gas: 725520)
Removal_mintBatch_multiple:test_32() (gas: 5769990)
Removal_mintBatch_multiple:test_4() (gas: 1061705)
Removal_mintBatch_multiple:test_8() (gas: 1734080)
Removal_mintBatch_revertsInvalidHoldbackPercentage:test() (gas: 56204)
Removal_mintBatch_reverts_mint_to_wrong_address:test() (gas: 88501)
Removal_mintBatch_zero_amount_removal:test() (gas: 310569)
Removal_mintBatch_zero_amount_removal:test() (gas: 310482)
Removal_mintBatch_zero_amount_removal_to_market_reverts:test() (gas: 86745)
Removal_multicall:test_balanceOfBatch() (gas: 490883)
Removal_release_listed:test() (gas: 485847)
Removal_release_listed_isRemovedFromMarket:test() (gas: 486166)
Removal_multicall:test_balanceOfBatch() (gas: 490796)
Removal_release_listed:test() (gas: 485812)
Removal_release_listed_isRemovedFromMarket:test() (gas: 486132)
Removal_release_partial_listed:test() (gas: 79202)
Removal_release_retired:test() (gas: 58836)
Removal_release_retired_2x:test() (gas: 60874)
Expand All @@ -146,8 +147,8 @@ Removal_setHoldbackPercentage:test_reverts_InvalidHoldbackPercentage() (gas: 291
RemovalQueue_getTotalBalanceFromRemovalQueue:test() (gas: 23808)
RemovalQueue_getTotalBalanceFromRemovalQueue:test_100xRemovalsOfTheDifferentVintages() (gas: 895673)
RemovalQueue_getTotalBalanceFromRemovalQueue:test_100xRemovalsOfTheSameVintage() (gas: 620208)
RemovalQueue_insertRemovalByVintage:test_insertRemovalOnce() (gas: 119635)
RemovalQueue_insertRemovalByVintage:test_insertRemovalTwice() (gas: 121191)
RemovalQueue_insertRemovalByVintage:test_insertRemovalOnce() (gas: 119590)
RemovalQueue_insertRemovalByVintage:test_insertRemovalTwice() (gas: 121125)
RestrictedNORI__validateSchedule:test_startTimeNotZero() (gas: 291)
RestrictedNORI__validateSchedule_reverts:test_restrictionDurationZero() (gas: 3384)
RestrictedNORI__validateSchedule_reverts:test_startTimeZero() (gas: 3384)
Expand All @@ -160,13 +161,14 @@ RestrictedNORI_scheduleExists:test() (gas: 15089)
RestrictedNORI_scheduleExists:test_doesntExist() (gas: 15068)
RestrictedNORI_transfers_revert:testSafeBatchTransferFromReverts() (gas: 29158)
RestrictedNORI_transfers_revert:testSafeTransferFromReverts() (gas: 23292)
Checkout_buyingFromOneRemoval:test() (gas: 696489)
Checkout_buyingFromOneRemoval_byApproval:test() (gas: 667292)
Checkout_buyingFromTenRemovals:test() (gas: 1869632)
Checkout_buyingFromTenRemovals_singleSupplier:test() (gas: 1858705)
Checkout_buyingFromTenRemovals_singleSupplier_byApproval:test() (gas: 1856705)
Checkout_buyingFromTenRemovals_singleSupplier_withoutFee:test() (gas: 1641040)
Checkout_buyingFromTenRemovals_withoutFee:test() (gas: 1651989)
Checkout_buyingFromTenSuppliers:test() (gas: 2747917)
Checkout_buyingWithAlternateERC20:test() (gas: 521010)
Checkout_buyingWithAlternateERC20_floatingPointPriceMultiple:test() (gas: 521010)
Checkout_buyingFromOneRemoval:test() (gas: 696245)
Checkout_buyingFromOneRemoval_byApproval:test() (gas: 666956)
Checkout_buyingFromTenRemovals:test() (gas: 1869384)
Checkout_buyingFromTenRemovals_singleSupplier:test() (gas: 1858424)
Checkout_buyingFromTenRemovals_singleSupplier_byApproval:test() (gas: 1856424)
Checkout_buyingFromTenRemovals_singleSupplier_withoutFee:test() (gas: 1641588)
Checkout_buyingFromTenRemovals_withoutFee:test() (gas: 1652550)
Checkout_buyingFromTenSuppliers:test() (gas: 2747669)
Checkout_buyingWithAlternateERC20:test() (gas: 520946)
Checkout_buyingWithAlternateERC20_floatingPointPriceMultiple:test() (gas: 520946)
Checkout_swapWithDifferentPermitSignerAndMsgSender:test() (gas: 696698)
4 changes: 4 additions & 0 deletions contracts/Errors.sol
View file
Expand Up @@ -129,3 +129,7 @@ error InvalidHoldbackPercentage(uint8 holdbackPercentage);
* contracts.
*/
error RemovalAlreadySoldOrConsigned(uint256 tokenId);
/**
* @notice Thrown when an ERC20 token transfer fails.
*/
error ERC20TransferFailed();
27 changes: 22 additions & 5 deletions contracts/Market.sol
View file
Expand Up @@ -1119,6 +1119,7 @@ contract Market is
from: 0,
to: countOfRemovalsAllocated
});
bool isTransferSuccessful;
uint8 holdbackPercentage;
uint256 restrictedSupplierFee;
uint256 unrestrictedSupplierFee;
Expand Down Expand Up @@ -1160,23 +1161,32 @@ contract Market is
removalId: removalIds[i]
});
}
_purchasingToken.transferFrom({
isTransferSuccessful = _purchasingToken.transferFrom({
from: from,
to: address(_restrictedNORI),
amount: restrictedSupplierFee
});
if (!isTransferSuccessful) {
revert ERC20TransferFailed();
}
}
}
_purchasingToken.transferFrom({
isTransferSuccessful = _purchasingToken.transferFrom({
from: from,
to: _noriFeeWallet,
amount: this.calculateNoriFee(removalAmounts[i])
});
_purchasingToken.transferFrom({
if (!isTransferSuccessful) {
revert ERC20TransferFailed();
}
isTransferSuccessful = _purchasingToken.transferFrom({
from: from,
to: suppliers[i],
amount: unrestrictedSupplierFee
});
if (!isTransferSuccessful) {
revert ERC20TransferFailed();
}
}
bytes memory data = abi.encode(
recipient,
Expand Down Expand Up @@ -1324,6 +1334,7 @@ contract Market is
from: 0,
to: countOfRemovalsAllocated
});
bool isTransferSuccessful;
uint8 holdbackPercentage;
uint256 restrictedSupplierFee;
uint256 unrestrictedSupplierFee;
Expand Down Expand Up @@ -1365,18 +1376,24 @@ contract Market is
removalId: removalIds[i]
});
}
_purchasingToken.transferFrom({
isTransferSuccessful = _purchasingToken.transferFrom({
from: from,
to: address(_restrictedNORI),
amount: restrictedSupplierFee
});
if (!isTransferSuccessful) {
revert ERC20TransferFailed();
}
}
}
_purchasingToken.transferFrom({
isTransferSuccessful = _purchasingToken.transferFrom({
from: from,
to: suppliers[i],
amount: unrestrictedSupplierFee
});
if (!isTransferSuccessful) {
revert ERC20TransferFailed();
}
}
bytes memory data = abi.encode(
recipient,
Expand Down
30 changes: 30 additions & 0 deletions contracts/test/MockUnsafeERC20Permit.sol
View file
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;
import "../IERC20WithPermit.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";

contract MockUnsafeERC20Permit is ERC20PermitUpgradeable, IERC20WithPermit {
function initialize() public initializer {
__ERC20Permit_init("MockUnsafeERC20Permit");
__ERC20_init_unchained("MockUnsafeERC20Permit", "MERC20");
_mint(_msgSender(), 1_000_000_000_000_000_000_000_000);
}

/**
* @dev "Unsafe" ERC20 implementations may not revert in the case of a transfer failure, and instead
* indicate failure by returning false. This contract can be used for testing purposes to simulate
* such an implementation.
*/
function transferFrom(
address,
address,
uint256
)
public
virtual
override(ERC20Upgradeable, IERC20Upgradeable)
returns (bool)
{
return false;
}
}
11 changes: 11 additions & 0 deletions docs/Errors.md
View file
Expand Up @@ -341,3 +341,14 @@ contracts.



## ERC20TransferFailed

```solidity
error ERC20TransferFailed()
```

Thrown when an ERC20 token transfer fails.




0 comments on commit 6a74176

Please sign in to comment.