tokenContractis always an unsafe input, for fairness, it is recommended to add a whitelist for tokenaddress(0)nonReentrant modifier should occur before all other modifiers@param is missing@return argument is missingif statements instead of logical AND (&&)address(this)inState modifier for high gas savingss_stateNatspec comments for smart contracts, interfaces and libraries.Submitted by ptsanev, carrotsmuggler, LaScaloneta, BenRai, TeamFliBit, prott00n, parsely, guhu, toshii, 0xdeth, aviggiano, ZedBlockchain, said017, chainNue, nican0r, Bernd, DevABDee, No12Samurai, BLACK PANDA REACH, 0xCiphky, innertia, Udsen, RiaZul, sonny2k, mau, radeveth, alexzoid, Tripathi. Selected submission by: nican0r.
Fee-on-transfer tokens aren't supported by the current escrow implementation.
In EscrowFactory::newEscrow L39 the price variable is used to transfer tokens from the buyer to the address at which the Escrow will be deployed:
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
for fee-on-transfer tokens the actual amount transferred to the Escrow address will be less than the price. This will cause the call to create a new instance of Escrow on L40 to trigger the following revert in the Escrow constructor L44:
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
buyers will therefore be unable to create an Escrow using fee-on-transfer tokens such as USDT.
The protocol prevents the use of fee-on-transfer tokens without explicitly defining these conditions. The inability to do so doesn't require malicious action by either party and given the sponsor comment with regard to compatible tokens this doesn't appear to be addressed:
"For the moment assume the following:
WETH, USDC, LINK, DAI
But, the buyer and seller could do whatever they want - just we would recommend against that."
Manual Review
Change the value passed in the instantiation of a new Escrow in EscrowFactory::newEscrow L41 to the balance of the Escrow address:
Escrow escrow = new Escrow{salt: salt}(
IERC20,
tokenContract.balanceOf(computedAddress),
msg.sender,
seller,
arbiter,
arbiterFee
);
Submitted by Cosine, serverConnected, 0xpathfindr, StErMi, darksnow, VanGrim, Madalad, 0xJuda, Bughunter101, Shogoki, 0xnevi, TeamFliBit, devanas, jnrlouis, rvierdiiev, Rolezn, pina, ABA, yakuzakiawe, toshii, pengun, SanketKogekar, Tatakae, leasowillow, 0xNiloy, Jarx, Yuki, 0xl3xx, aviggiano, rajat, ravikiranweb3, DevABDee, 0xumarkhatab, tsvetanovv, sz41th, chainNue, Bernd, alliums0517, Modey, 0xdeadbeef, 0x4ka5h, 0xANJAN143, tsar, Juntao, hash, 0xSwahili, dimakush, Maroutis, ayeslick, devtooligan, 0xhacksmithh, XVIronSec, serialcoder, AcT3R, theOwl, BAHOZ, VictoryGod, Avci, honeymewn, Tripathi, 0xPublicGoods, iroh. Selected submission by: 0xnevi.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L103
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L109
If there is no arbiter, buyer can never retract the funds sent to escrow, causing tokens to be lost forever.
In Escrow.initiateDispute(), if no arbiter is set by buyer, dispute can never be initiated.
function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {
/// @audit if no arbiter set, dispute cannot be initiated
-> if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();
s_state = State.Disputed;
emit Disputed(msg.sender);
}
The only way to retrieve back the funds is through buyer/seller first calling initiateDispute() then arbiter calling resolveDispute(). However, Escrow.resolveDispute() will always revert due to the inState modifier because initiateDispute() cannot be called to set state of escrow to Dispute.
/// @inheritdoc IEscrow
function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
/// @audit this function will always revert unless initiateDispute is called by buyer or seller
uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
if (totalFee > tokenBalance) {
revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
}
s_state = State.Resolved;
emit Resolved(i_buyer, i_seller);
if (buyerAward > 0) {
i_tokenContract.safeTransfer(i_buyer, buyerAward);
}
if (i_arbiterFee > 0) {
i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
}
tokenBalance = i_tokenContract.balanceOf(address(this));
if (tokenBalance > 0) {
i_tokenContract.safeTransfer(i_seller, tokenBalance);
}
}
In the above scenario, the only way to transfer funds out of escrow is for buyer to call confirmReceipt() and send funds to seller.
Consider the scenario where there is no arbiter, and buyer is dissatisfied with seller delivery, but escrow contract is created with no arbiter.
In this case, there is no way for buyer to retrieve their funds sent to Escrow contract since both functions initiateDispute() and resolveDispute() cannot be called if there are no arbiter set as their tokens are locked forever in Escrow contract, unless they are willing to go ahead with payment via confirmReceipt().
Manual Analysis
Consider adding an additional onlyBuyer function where withdrawal of escrowed funds by buyer is allowed when there is no arbiter set. However, set a delay so that buyer cannot immediately pull escrowed tokens to grief sellers payment.
In general start of audit to end of audit will normally not take more than 3 months.
Submitted by yixxas, carrotsmuggler, owl, LaScaloneta, Breeje, TeamFliBit, jprod15, Polaristow, Vagner, rvierdiiev, crippie, 0xStalin, Bauchibred, caventa, 0xJuda, cccz, auditsea, 33audits, RugpullDetector, simon, tsvetanovv, said017, 0xhals, talfao, Bernd, MrjoryStewartBaxter, Tricko, Lokacho, AlexCzm, 0xCiphky, tsar, 0xMosh, pks27, KupiaSec, ayeslick, golanger85, Udsen, theOwl, ADM, Avci, 0xScourgedev, sonny2k. Selected submission by: TeamFliBit.
It is stated in the readme and by Patrick Collins that the protocol will only accept vetted tokens such as USDC, DAI, WETH, etc..
Yet by using these popular tokens such as USDC, there exists a case where the funds for every participant will locked permanently. This is due to the blacklist system which is implemented by USDC and many other popular well-reputed tokens.
in Escrow.sol, if any of the participants is blacklisted (due to illegal activities, using Tornado Cash for privacy reasons, spite by one of the participants), the funds for every participant will be locked permanently.
In the Escrow.sol contract, when an escrow is created, there are two ways to "get the funds out".
buyer calls the function confirmReceipt() which will transfer the entire contract token balance to the seller address.buyer or the seller can call initiateDispute() and the arbiter can call the function resolveDispute which will transfer part of the funds to the buyer address, a fee to the arbiter address and finally, the potential remaining funds in the contract to the seller addressIf one the participant's addresses that is set to receive funds is blacklisted however, the functions will revert and essentially "brick" the contract, making it impossible to ever recover the funds.
Blacklisting is certainly not uncommon and is used many of the popular token used for payments, such as the stablecoin USDC. An address can get blacklisted for illegal activities, some were blacklisted for just using tools like Tornado Cash to keep their transactions private and it is also perfectly possible for a disgruntled participant to intentionally blacklist his address to block the withdrawal of funds.
Essentially, if any of the addresses involved is blacklisted, none of the participants can receive their funds.
We have included a POC to showcase how it works in all the cases. You can get the POC file in the following gist: https://gist.github.com/TheNaubit/b0cc2e6b4d1ae2bea637d9d89d9b5b19
To run it, has paste the content of the gist inside a file called WithdrawFailBlacklisted.t.sol inside the test folder in the project. Then run them with the following command:
forge test --match-contract WithdrawFailBlacklisted
Affected code:
For reference, there are other contests with similar findings like:
When this issue happens, all the funds in the contract are locked forever making every participant to lose their funds.
Manual review & Foundry
There are one solution with two parts:
withdraw function only callable when the escrow is finished where each participant can withdraw the funds they own. In this case, if any participant is blacklisted, at least the rest will be able to get the rest of the funds.i_arbiterFee can prevent paymentSubmitted by ptsanev, 0xdeadbeef, 0xJuda, mahdiRostami, guhu, pontifex, aviggiano, cccz, 0xNiloy, ZedBlockchain, chainNue, 0xPinto, golanger85, nervouspika, Shogoki. Selected submission by: 0xdeadbeef.
i_arbiterFee is a fixed value and can brick payment in resolution of disputes if the payment token has a rebasing balance.
Instead, i_arbiterFee should be a percentage and should the actual fee should be based on the current balance of the contract.
function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
if (totalFee > tokenBalance) {
revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
}
s_state = State.Resolved;
emit Resolved(i_buyer, i_seller);
if (buyerAward > 0) {
i_tokenContract.safeTransfer(i_buyer, buyerAward);
}
if (i_arbiterFee > 0) {
i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
}
tokenBalance = i_tokenContract.balanceOf(address(this));
if (tokenBalance > 0) {
i_tokenContract.safeTransfer(i_seller, tokenBalance);
}
}
As can be seen above, there are three values that are sent:
buyerAward- controlled by the arbiter, the refund that the buyer will receivei_arbiterFee - predefined fixed value that the arbiter will receivetokenBalance - the remaining of the the contract will be sent to the sellerin case i_tokenContract is a token that has a rebasing balance. i_arbiterFee can be bigger then the current balance and resolveDispute will revert in the following statement
if (totalFee > tokenBalance) {
revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
}
Note that it is popular to use rebasing tokens. Additionally, it is common that projects (buyers) will request the payouts in their own token (which can be rebasing).
Funds can be locked in the Escrow contract due to rebasing
Manual
Instead of setting a fixed i_arbiterFee either calculate the percentage at the escrow deployment or set the percentage directly.
This will also require to change resolveDispute to send a percentage of the balance to arbiter instead of a fixed payment
Escrow should make sure that buyer, seller, arbiter are different from each other.Submitted by 0xdraiakoo, 0xpathfindr, StErMi, owl, jonatascm, 0xSafeGuard, Madalad, LaScaloneta, 0xJuda, 0x70C9, kodyvim, nisedo, Shogoki, Cosine, crippie, BenRai, devanas, 0xnevi, jigster, niluke, Bauchibred, Rolezn, castleChain, asuiTouthang, Tatakae, ZedBlockchain, neocrao, 0xNiloy, ro1sharkm, RugpullDetector, JohnnyTime, zuhaibmohd, ravikiranweb3, rajat, y51r, 0xhals, talfao, MrjoryStewartBaxter, pep7siup, qbs, 0x4ka5h, xAlismx, InAllHonesty, 0xCiphky, 0xMosh, 0xPinto, innertia, Rackslabs academy, Udsen, radeveth, klaus, 0xmuxyz, SolSaver. Selected submission by: RugpullDetector.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L32-L52
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) {
if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();
if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
i_price = price;
i_tokenContract = tokenContract;
// @audit - it does not check if buyer != sellers and buyer != arbiter
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;
i_arbiterFee = arbiterFee;
}
Unfair arbitration might be possible or malicous auditor might abuse to increase his auditing profile.
Manual Review
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) {
if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();
if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
i_price = price;
i_tokenContract = tokenContract;
// @audit - it does not check if buyer != sellers and buyer != arbiter
+ if (buyer == arbiter) revert Escrow__InvalidAddress();
+ if (seller == arbiter) revert Escrow__InvalidAddress();
+ if (buyer == seller) revert Escrow__InvalidAddress();
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;
i_arbiterFee = arbiterFee;
}
Submitted by RugpullDetector, BAHOZ. Selected submission by: BAHOZ.
[L] If the arbiter is not set, arbiter fee should not be positive
In the constructor of Escrow.sol, unlike other params like buyer, seller and tokenContract; arbiter is not required to be nonzero. The reason for this is the possibility to create an escrow without an arbiter.
However if this is the case, there is no check that ensure if the arbiterFee equals to zero which allows a weird state where there is no arbiter but there is a positive arbiterFee.
Since without an arbiter it is not possible to initiateDispute(), the issue does not cause any problems exept from a faulty state where there is a positive arbiterFee without an arbiter
Manual Review
If the arbiter equals to address(0), require arbiterFee to be equal to 0.
Submitted by sm4rty, 0xsandy. Selected submission by: sm4rty.
The resolveDispute function in the contract does not include all of the relevant information in the Resolved event.
The resolveDispute function does not include all of the relevant information in the Resolved event. The event emits the buyer and seller addresses. However, it does not include any other information, such as the buyer award, the arbiter fee, or the amount paid to the buyer.
The lack of emission of critical parameters in the event could affect users with the front-end, as not all data will be shown there. Additionally, external observers or off-chain systems may not have access to critical information about the resolution of the dispute.
Manual Analysis
Consider adding the following information in the event emitted.
event Resolved(
address indexed buyer,
address indexed seller,
uint256 buyerAward,
uint256 arbiterFee,
uint256 amountPaidToBuyer
);
tokenContractis always an unsafe input, for fairness, it is recommended to add a whitelist for tokenSubmitted by Bughunter101, 0xnevi, 0xdeth, ZedBlockchain, simon, chainNue, 0xDanielH, 0xVinylDavyl, Rackslabs academy, 0xbugalba, Avci, 0xSCSamurai. Selected submission by: Bughunter101.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L20
As the code comment says: @dev There is a risk that if a malicious token is used, the dispute process could be manipulated.
I do not agree to hand over the legitimacy and security of the tokenContract to msg.sender. So what should the protocol do to limit.
tokenContractis always an unsafe input, for fairness, it is recommended to add a whitelist for token
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L20
As the code comment says: @dev There is a risk that if a malicious token is used, the dispute process could be manipulated.
vs code
I think it's a design issue. tokenContractis always an unsafe input, for fairness, it is recommended to add a whitelist for token, and add a function to add token to whitelist by owner.
Submitted by 0xBorgia, T1MOH, Shogoki, ZedBlockchain, JohnnyTime, ubermensch, 33audits, Omsify, Arz, Rackslabs academy, honeymewn, hash. Selected submission by: JohnnyTime.
Using clones, also known as minimal proxies, for deploying escrow contracts through the factory contract offers substantial gas savings. Clones are small and inexpensive smart contracts that delegate calls to an implementation contract, eliminating the need to deploy the entire contract repeatedly for each buyer. Instead, lightweight proxies are created, pointing to the shared contract logic. This approach reduces gas costs significantly, making the deployment process more efficient and cost-effective, especially when deploying multiple escrow contracts over time.
Clones, also known as minimal proxies, are small and cost-effective smart contracts described in ERC1167. They delegate all incoming calls to an implementation (template) contract that contains the required functionality. The address of this implementation contract is directly stored in the contract code, eliminating the need for additional storage loads (sload).
The use of clones can significantly reduce the gas cost of deploying Escrow contracts. Instead of deploying the entire Escrow contract with all its logic for each buyer, only a minimal proxy pointing to the escrow contract logic needs to be deployed. This proxy acts as a lightweight representative of the actual contract, efficiently delegating calls to the underlying implementation contract.
Saving a lot of gas in every Escrow contract deployment.
VSCode
To optimize the deployment process, the EscrowFactory.sol contract should be modified to employ OpenZeppelin clones, creating cost-effective versions instead of deploying the entire Escrow.sol logic for each instance
Use the tutorial to learn how to work with minimal proxies / clones:
https://blog.openzeppelin.com/workshop-recap-cheap-contract-deployment-through-clones
FYI: You can use the cloneDeterministic and predictDeterministicAddress functions to predict the deployed escrow address before deploying it.
Submitted by Madalad, zaevlad, pengun, ChainSentry, aviggiano, ZedBlockchain, qckhp, SecurityDev23, chainNue, BLACK PANDA REACH, Juntao, VictoryGod. Selected submission by: qckhp.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L20
When setting up the Escrow Contract there could be a deadline for processing a dispute, me as an auditor would have more trust providing the services, for the escrowed amount.
The buyer could set a deadline for dispute process, so in case the arbiter does not act in time the seller would still have access to the funds.
Putting more pressure on the arbiter for resolving a dispute/lock of funds
Manual review
Add a optional disputeDeadline parameter when setting up the Escrow contract. And if the deadline passed without resolve, allow the seller to claim the amount.
Submitted by Shogoki, TeamFliBit, jboetticher, karanctf. Selected submission by: TeamFliBit.
The error Escrow__FeeExceedsPrice is incorrectly named.
Escrow.sol
43 if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
The checks throw an error if the arbiterFee is greater or equal than the price. However, If the arbiterFee is equal to price (arbiterFee = price), then the naming of the error thrown is incorrect since it does not exceed price.
QA
Manual review
Rename the error to Escrow_FeeEqualsOrExceedsPrice
Submitted by LaScaloneta.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L7-L9
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L7-L8
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L4
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrowFactory.sol#L5
Solmate's implementation of the libraries used on the protocol are more optimized saving gas to the user. The changes on the gas consumption could be seen below:
Test result: ok. 25 passed; 0 failed; 0 skipped; finished in 118.07ms testSameSaltReverts() (gas: -3184 (-0.000%)) testConstructorBuyerZeroReverts() (gas: -109 (-0.184%)) testConstructorTokenZeroReverts() (gas: -109 (-0.185%)) testRevertsIfTokenTxFails() (gas: -6468 (-0.857%)) testRevertIfFeeGreaterThanPrice() (gas: -1472 (-1.044%)) testSellerZeroReverts() (gas: -1472 (-1.049%)) testConfirmReceiptRevertsOnTokenTxFail() (gas: -109687 (-7.714%)) testResolveDisputeWithBuyerAward() (gas: -103808 (-12.229%)) testResolveDisputeTransfersTokens() (gas: -103808 (-12.235%)) testResolveDisputeZeroSellerTransfer() (gas: -103208 (-12.866%)) testResolveDisputeChangesState() (gas: -103201 (-12.934%)) testCanOnlyResolveInDisputedState() (gas: -102569 (-13.244%)) testCanOnlyConfirmInCreatedState() (gas: -102561 (-13.363%)) testCanOnlyInitiateDisputeInConfirmedState() (gas: -102575 (-13.384%)) testTransfersTokenOutOfContract() (gas: -102575 (-13.386%)) testResolveDisputeFeeExceedsBalance() (gas: -101977 (-13.386%)) testConfirmReceiptEmitsEvent() (gas: -102575 (-13.395%)) testStateChangesOnConfirmedReceipt() (gas: -102575 (-13.397%)) testCreatingEscrowEmitsEvent() (gas: -103830 (-13.408%)) testInitiateDisputeEmitsEvent() (gas: -101974 (-13.436%)) testInitiateDisputeChangesState() (gas: -101974 (-13.449%)) testCreatingEscrowHasBuyerActuallyBeBuyer() (gas: -101974 (-13.533%)) testComputedAddressEqualsDeployedAddress() (gas: -102511 (-13.583%)) testDeployEscrowFromFactory() (gas: -101974 (-13.818%)) testOnlyArbiterCanCallResolveDispute(address) (gas: -101974 (-13.822%)) testInitiateDisputeWithoutArbiterReverts() (gas: -101974 (-13.838%)) testConfirmReceiptOnlyByBuyer() (gas: -101976 (-13.853%)) testOnlyBuyerOrSellerCanCallinitiateDispute(address) (gas: -101974 (-13.867%)) testConfirmReceiptOnlyByBuyerFuzz(address) (gas: -101975 (-13.874%)) testTokenZeroReverts() (gas: 55174 (234.056%)) Overall gas change: -2322869
Manual Review
Change OZ to Solmate on every file.
Submitted by ptsanev, akhilmanga, caventa, codeslide, 0xNiloy, ro1sharkm, castleChain, mahyar, 0xMosh, xfu, 0xPublicGoods, ni8mare. Selected submission by: ro1sharkm.
The Escrow contract contains a vulnerability that allows users to bypass the fund check during deployment by providing a price of 0. The bug circumvents the intended behavior of verifying that funds have been deposited to the contract's address before initialization.
The Escrow contract's constructor includes a condition to ensure that tokens have been sent to this address during the contract deployment
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
If the user sets the price to 0, the condition becomes ineffective, as 0 is not greater than any balance, including 0. Consequently, the fund check is bypassed, allowing users to deploy the contract without sending any funds.
It violates the expected functionality of the escrow system. The severity of this bug is classified as "Low" because it does not directly compromise the security of the contract or user funds
Manual analysis
price>0
Submitted by Bad, Stoicov, 0xnevi, souilos, ChainSentry, oualidpro, rajat, 33audits, SMA4, TheSavageTeddy, emrekocak, 0xhacksmithh, Udsen, SAQ, alexzoid, Avci. Selected submission by: Bad.
When deploying a new Escrow via the factory, we can check if the set price is not 0 before interacting with the IERC20 via external calls. This ultimately saves the user some gas.
As it stands, the factory contract performs the token transfer regardless of whether the price is 0 or not. There are currently no checks to ensure that price is always greater than 0, so it can be assumed that a price of 0 is a legal choice (please see my other finding that states a logic flaw resulting in a DoS for the user).
If price is set to 0, the IERC20 interaction will still occur and ultimately waste some gas for the caller. There are 2 interactions during escrow creation that can be tweaked to reduce the overall cost:
EscrowFactory.sol:newEscrow()
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow.sol:constructor()
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
A quick 'forge test --gas-report' shows the following (extracted from the overall results) for the code before the patch:
| src/EscrowFactory.sol:EscrowFactory contract | | | | | |
|----------------------------------------------|-----------------|--------------------|--------|---------------------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1720216 | 8620 | | | | |
| Function Name | min | avg | median | max | # calls |
| computeEscrowAddress | 11014 | 11014 | 11014 | 11014 | 2 |
| newEscrow | 12324 | 288303014855889986 | 627054 | 8937393460515961103 | 31 |
Additional gas costs occur even if the price is set to 0. These can be avoided.
This can be addressed by simply wrapping the two identified called with a "not zero" check. In some cases, using != instead of > for the 0 check can also be more efficient. These have been used in this remediation example:
EscrowFactory.sol:newEscrow()
if (price != 0) {
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
}
Escrow.sol:constructor()
if (price != 0){
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
}
The following shows the same gas report after the patch, as can be seen, the majority of results are reduced:
| src/EscrowFactory.sol:EscrowFactory contract | | | | | |
|----------------------------------------------|-----------------|--------------------|--------|---------------------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1726423 | 8651 | | | | |
| Function Name | min | avg | median | max | # calls |
| computeEscrowAddress | 11014 | 11014 | 11014 | 11014 | 2 |
| newEscrow | 12349 | 288303014855889527 | 627114 | 8937393460515961046 | 31 |
address(0)Submitted by Bad, 0xbug, Dharma, oualidpro, 0xl3xx, codeslide, neocrao, ch0bu, Ericselvig, SMA4, TheSavageTeddy, xfu, 0x11singh99, 0xhacksmithh, SAQ, alexzoid, radeveth, SolSaver. Selected submission by: xfu.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol#L40
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol#L41
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol#L42
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol#L103
Inline Assembly more gas efficient and Saving Gas with Simple Inlining.
There are 4 instances of this issue:
address(tokenContract) == address(0) should use assembly to check for address(0)
buyer == address(0) should use assembly to check for address(0)
seller == address(0) should use assembly to check for address(0)
i_arbiter == address(0) should use assembly to check for address(0)
save gas
Use assembly to check for address(0):
function addrNotZero(address _addr) public pure {
assembly {
if iszero(_addr) {
mstore(0x00, "zero address")
revert(0x00, 0x20)
}
}
}
Submitted by Bad, 0xsandy. Selected submission by: 0xsandy.
Enum is used in Escrow contract for managing various states of functions. But Enums are gas expensive and it can be done cheaper by using constants. One of the examples of using constants instead of enums is Openzeppelin's ReentrancyGuard.sol contract.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol
It costs so much less gas to read and write constant values than reading and writing from Enums.
Savings for confirmReceipt function:
| Average | Median | Max | |
|---|---|---|---|
| Before | 24518 | 40322 | 40322 |
| After | 12942 | 22770 | 22770 |
Savings for initiateDispute function:
| Average | Median | Max | |
|---|---|---|---|
| Before | 16855 | 23603 | 23603 |
| After | 1280 | 1657 | 1657 |
Savings for resolveDispute function:
| Average | Median | Max | |
|---|---|---|---|
| Before | 27017 | 22342 | 62560 |
| After | 26717 | 21799 | 62419 |
Manual Analysis and Gas Savings are calculated via $ forge test --gas-report
Implementing constants in place of Enum in Escrow contract requires quiet a bit of refactoring in Escrow.sol, IEscrow.sol and EscrowTest.t.sol. But for testing purposes, I have already modified all these contracts and they can be found here: https://github.com/0xSandip/EscrowGasChanges
Basically what i did was:
/Escrow.sol
uint256 private constant CREATED = 1;
uint256 private constant CONFIRMED = 2;
uint256 private constant DISPUTED = 3;
uint256 private constant RESOLVED = 4;
/Escrow.sol
uint256 private s_state;
s_state as CREATED in the constructor:/Escrow.sol
constructor(
...
) {
...
s_state = CREATED;
}
inState modifier:/Escrow.sol
modifier inState(uint256 expectedState) {
if (s_state != expectedState) {
revert Escrow__InWrongState(s_state, expectedState); //error also changed for uint256
}
_;
}
and modified all the instances of the state and values everywhere in the Escrow, IEscrow and EscrowTest.t.sol contract which can be referenced from the above link.
nonReentrant modifier should occur before all other modifiersSubmitted by Stoicov, Rolezn, oualidpro, 0xl3xx, neocrao, ch0bu, 0xPinto, mau, owade, SolSaver. Selected submission by: SolSaver.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol#112
The nonReentrant modifier should occur before all other modifiers
This is a best-practice to protect against reentrancy in other modifiers
Instances (1):
File: src/Escrow.sol
112: function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
Manual Code Review
Add nonReentrant modifier before all other modifiers
Submitted by owl.
both resolveDispute and confirmReceipt as well as the constrcturor utilize similar code i_tokenContract.balanceOf(address(this)), make it a function to reduce Repetition
--
improve readability
manual review
add function getContractBalance that would be used in place of the duplicate code
Submitted by owl.
additional import statement can be removed and instead import directly from SafeERC20 since it is present there
N/A
N/A
manual review
remove IERC20 import statement and instead import it from SafeERC20
import {SafeERC20, IERC20}
Submitted by StErMi, ZedBlockchain, ABA, 0xsandy. Selected submission by: ABA.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L41-L42
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L19
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L7
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrowFactory.sol#L14
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrowFactory.sol#L8-L10
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L17
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L13
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L28
There are Incomplete, incorrect or ambiguous comments in the code. Observation, the following instances were not mentioned in the bot analysis.
in IEscrow::resolveDispute
change the natspec comment:
Arbiter can resolve dispute and claim token reward by entering in split of
pricevalue minusarbiterFeeset at construction.
to a more understandable phrase, example: Arbiter can resolve a dispute and claim its token reward by providing the split value for buyer and seller, minus his arbiterFee which is set at construction.
in IEscrowFactory
ie to i.e.in EscrowFactory
... to spend the price amount before ... is ambiguous; change it to ... to spend the token price amount before ...in Escrow
an: add it before optional: and an optional arbiterand before the last element of the enumeration; add it: arbiter and arbiterFee.Submitted by dacian.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L94-L99
Missing partial payment option does not reflect real-world usage of private auditors & protocols, limiting usefulness of the Escrow contract.
In private audits it is common for the buyer (protocol team) to pay a deposit (a % of the total fee, typically 30-50%) before the audit is started, and the remainder of the fee is paid once the audit findings are delivered. A smaller down-payment can also be paid to secure the auditor's time which may be forfeited if the protocol team then decides to cancel the audit.
The Escrow contract does not offer this; it is "all or nothing", either the buyer pays the entire sum or they pay nothing.
The Escrow contract may not be suitable for its intended purpose; auditors in particular may not use it due to not receiving a down-payment before beginning the audit.
Manual
The Escrow contract should be created with a downPayment parameter and the state machine expanded to allow auditors to receive an agreed-upon down-payment before starting the audit.
Submitted by jboetticher.
ReentrancyGuard.sol is a very simple abstract contract. Its functionality can be baked into Escrow.sol, and its gas can be optimized as well.
Over 800 gas can be saved between deployments.
Remove Escrow's inheritance of ReentrancyGuard and add the following:
contract Escrow is IEscrow {
// ...rest of the smart contract
/////////////////////
// Bake Reentrancy
/////////////////////
uint256 private _status;
modifier nonReentrant() {
_nonReentrantBefore();
_;
_nonReentrantAfter();
}
error Escrow__ReentrantCall();
function _nonReentrantBefore() private {
if(_status == 1) revert Escrow__ReentrantCall();
_status = 0;
}
function _nonReentrantAfter() private {
_status = 0;
}
}
The Escrow__ReentrantCall error can be moved into IEscrow.sol for continuity.
@param is missingSubmitted by 0xbug, Rolezn, oualidpro, codeslide, neocrao, TheBlockChainer, Dharma, Proxy, radeveth, SolSaver. Selected submission by: neocrao.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/EscrowFactory.sol
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/IEscrow.sol
Instances (3):
File: src/Escrow.sol
32: constructor(
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
File: src/EscrowFactory.sol
57: function computeEscrowAddress(
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/EscrowFactory.sol
File: src/IEscrow.sol
45: function resolveDispute(uint256 buyerAward) external;
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/IEscrow.sol
Custom analyzer tool
Add NatSpec @param to make the documentations complete
@return argument is missingSubmitted by 0xbug, Rolezn, oualidpro, codeslide, neocrao, TheBlockChainer, radeveth, SolSaver. Selected submission by: SolSaver.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/EscrowFactory.sol
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/IEscrow.sol
NatSpec @return argument is missing
Instances (15):
File: src/Escrow.sol
144: function getPrice() external view returns (uint256) {
148: function getTokenContract() external view returns (IERC20) {
152: function getBuyer() external view returns (address) {
156: function getSeller() external view returns (address) {
160: function getArbiter() external view returns (address) {
164: function getArbiterFee() external view returns (uint256) {
168: function getState() external view returns (State) {
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
File: src/EscrowFactory.sol
57: function computeEscrowAddress(
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/EscrowFactory.sol
File: src/IEscrow.sol
51: function getPrice() external view returns (uint256);
53: function getTokenContract() external view returns (IERC20);
55: function getBuyer() external view returns (address);
57: function getSeller() external view returns (address);
59: function getArbiter() external view returns (address);
61: function getArbiterFee() external view returns (uint256);
63: function getState() external view returns (State);
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/IEscrow.sol
Manual Code Review
Use NatSpec @return
Submitted by 0xbug, oualidpro, neocrao, TheBlockChainer, SolSaver. Selected submission by: SolSaver.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
Constants in comparisons should appear on the left side
Doing so will prevent typo bugs
Instances (6):
File: src/Escrow.sol
41: if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
42: if (seller == address(0)) revert Escrow__SellerZeroAddress();
106: if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();
128: if (buyerAward > 0) {
131: if (i_arbiterFee > 0) {
135: if (tokenBalance > 0) {
Link to code - https://github.com/Cyfrin/2023-07-escrow/tree/main/src/Escrow.sol
Manual Code Review
Use constants on the left side
Submitted by BenRai, devtooligan. Selected submission by: devtooligan.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L22-L23
The addresses for the seller and arbiter are set once upon contract creation. If a mistake is made in the address of either the seller or the arbiters, all funds could be lost.
Both i_seller and i_arbiter are immutables set during the constructor. Additionally, the contract must be funded at or before the time of creation.
address private immutable i_seller;
address private immutable i_arbiter;
Low. The severity of this is high because the entire contract balance would be lost if a mistake was made in the seller address and there was no arbiter. If a mistake was made in the arbiter address, funds would be lost if the arbiter was needed during the escrow.
The difficulty of triggering this bug is also very high. Although anyone can create an escrow contract, they would have to make a mistake in the address. Because the difficulty is so high, this is a low impact finding in spite of the high severity.
Manual review
Use a two-step process for setting both the seller and arbiter. In the first step, the buyer will propose an address. If the proposed address is not accepted (for example because of a mistake in the address) then the buyer can set a new pending address. Forward immutability can be maintained by adding logic that an address cannot be changed after it is set.
This will increase the runtime gas cost for any functions that read i_seller or i_arbiter but the functions are not commonly called and the security benefits from this mitigation are large.
Submitted by B353N, toshii, neocrao, tsar. Selected submission by: neocrao.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol
Add methods to add arbiter in existing Escrow contracts
If the Escrow contract gets created without the arbiter, and if an arbiter is needed to settle a dispute, then there is no way to add one.
The only way to get funds our of the contract is:
confirmReceipt() method to send funds to the sellerFor Case 2, the funds cannot be taken out of the contract in case of disputes, and so the funds get locked in there.
Marking this as medium as both the following medium criteria satisfy:
Source: https://docs.codehawks.com/rewards-and-judging
Manual analysis
Create a method that can update the arbiter if needed. This method can be written such that both the buyer and seller agree to add the arbiter based on their votes.
if statements instead of logical AND (&&)Submitted by jnrlouis, Dharma, oualidpro, ihtishamsudo, Aarambh, Arz, SMA4, 0xgrbr, hasanza, 0x11singh99, 0xhacksmithh, SAQ. Selected submission by: hasanza.
Compared to nested if statements, using logical AND (&&) results in a relatively larger deployment size, and higher gas overhead on each call.
Inside the onlyBuyerOrSeller modifier, a logical AND (&&) has been used to ensure both conditions (that caller is either buyer or seller) are met for the modified function to be accessed. However, using a logical AND is more expensive than using a nested if statement. This is due to how the compiler optimizes for nested if statements.
/// @dev Throws if called by any account other than buyer or seller.
modifier onlyBuyerOrSeller() {
if (msg.sender != i_buyer && msg.sender != i_seller) {
revert Escrow__OnlyBuyerOrSeller();
}
_;
}
Gas
Forge, Foundry Tooklit (gas snapshots and gas reports)
The modifier can be refactored to use a nested if statement:
/// @dev Throws if called by any account other than buyer or seller.
modifier onlyBuyerOrSeller() {
if (msg.sender != i_buyer) {
if (msg.sender != i_seller) {
revert Escrow__OnlyBuyerOrSeller();
}
}
_;
}
With this change, the following gas savings were seen:
~1800 gas was saved in each Escrow test in EscrowTest.t.sol, whereas a cumulative 41650 gas saving was seen across all tests.3666 to 3657 (a saving of 9 bytes).591900 to 590100 (a saving of 1800 gas).initiateDispute (the function using the modifier in question)'s size reduced from 316 bytes to 292 bytes (24 bytes). Whereas calling the function saved 19 gas on each call.address(this)Submitted by jnrlouis, castleChain, codeslide, SMA4, TheSavageTeddy, 0xhacksmithh, SAQ, honeymewn. Selected submission by: codeslide.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L44
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L98
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L110
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L125
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L30
Instead of using address(this), it is more gas-efficient to pre-calculate and use the predefined address. Foundry's script.sol and Solmate's LibRlp.sol contracts can help pre-determine the address (see computeCreateAddress). The address can be passed in via a constructor argument and assigned to an immutable variable (rather than using a hardcoded constant) so that the code can remain the same across deployments on different networks.
There are 5 instances of this issue.
File: src/Escrow.sol
44: if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
98: i_tokenContract.safeTransfer(i_seller, i_tokenContract.balanceOf(address(this)));
110: uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
125: tokenBalance = i_tokenContract.balanceOf(address(this));
| File Link | Instance Count | Instance Links |
|---|---|---|
| Escrow.sol | 4 | 44,98,110,125 |
File: src/EscrowFactory.sol
30: address(this),
| File Link | Instance Count | Instance Link |
|---|---|---|
| EscrowFactory.sol | 1 | 30 |
250 gas
baudit: a custom static code analysis tool; manual review
Use predefined address instead of address(this).
Submitted by hunterw3b, bronzepickaxe, ADM, 0xsandy, Udsen, BAHOZ, cuthalion0x. Selected submission by: 0xsandy.
The contract Escrow.sol uses reentrancy guard contract and nonReentrant modifier to not allow someone to reenter into the same function twice but it is not required as inState modifier ensures that a function can only be called by a particular State type. And all of these State types are updated before any external calls so, Reentrancy is not possible. So, they can be removed to save some gas.
The contract Escrow inherits reentrancy guard and function resolveDispute uses nonReentrant modifier which is not necessary and can be removed. It can save up to 276 gas for resolveDispute function and 40525 gas in deployment cost.
Gas Savings for resolveDispute function:
| Average | Median | Max | |
|---|---|---|---|
| Before | 27017 | 22342 | 62460 |
| After | 26794 | 22094 | 62184 |
Deployment cost and deployment size:
| Deployment cost | Deployment size | |
|---|---|---|
| Before | 591900 | 3666 |
| After | 551375 | 3569 |
Manual Analysis and Gas savings are calculated using $ forge test --gas-report
Don't inherit from reentrancy guard and don't use nonReentrant modifier in the resolveDispute function.
inState modifier for high gas savingsSubmitted by Dharma, hunterw3b, larsson, Ericselvig, SMA4, hasanza, InAllHonesty, SAQ. Selected submission by: hasanza.
Instead of housing the logic in a modifier, moving it into an internal function, and making a call to that function inside the modifier, can save a lot of gas.
The modifier inState is used thrice. As such, its code is copied over to each modified function and is duplicated three times. If, instead, we create an internal view function with the state check, and invoke that function inside the modifier, we can save a lot of gas. What this does is that instead of the actual check being copied over in each modified function, just an invocation to the internal function is made.
Gas
Forge, Foundry Toolkit (gas report, gas snapshots)
Use the inState modifier in conjunction with an internal _inState function to reduce contract size and save on gas. Only the following change needs to be made:
modifier inState(State expectedState) {
_inState(expectedState);
_;
}
function _inState(State expectedState) internal view {
State currentState = s_state;
if (currentState != expectedState) {
revert Escrow__InWrongState(currentState, expectedState);
}
}
With this change in place, a gas saving of ~13000 gas was seen on each test through the gas snapshot. A total gas reduction of 303596 was seen across all tests. In addition, the deployment size for Escrow.sol reduced by 5 bytes and deployment cost reduced by 13012 gas. Saving ~13K gas on each deployment adds up to significant savings in the long-term.
Submitted by Dharma, oualidpro, 0xl3xx, InAllHonesty, SAQ. Selected submission by: InAllHonesty.
Non-strict inequalities (>=) are cheaper than strict ones (>). Some checks from resolveDispute in Escrow.sol could be optimized as follows:
- if (buyerAward > 0) {
+ if (buyerAward >= 1) {
i_tokenContract.safeTransfer(i_buyer, buyerAward);
}
- if (i_arbiterFee > 0) {
+ if (i_arbiterFee >= 1) {
i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
}
tokenBalance = i_tokenContract.balanceOf(address(this));
- if (tokenBalance > 0) {
+ if (tokenBalance >= 1) {
i_tokenContract.safeTransfer(i_seller, tokenBalance);
}
s_stateSubmitted by pina.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L26
The declared variable s_state has no information about the initial state and in the constructor it is not initialized.
When the contract is implemented, the initial value for the s_state is 0 for default, but it's not very clear to know that.
Only the knowledgeable developer would know that when it is a uint256 value type and is not initialized, the default value is 0.
Users or developers will not see as clearly the value of s_state
Manual code review
Add this comment:
+/// @dev The initial value for s_state is 0 which corresponds to the `Created` state.
State private s_state;
Submitted by oualidpro, Arz, mgf15. Selected submission by: oualidpro.
https://github.com/Cyfrin/2023-07-escrow/tree/main//src/EscrowFactory.sol#L51
Events may be emitted out of order due to reentrancy
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls
File: /src/EscrowFactory.sol
51: emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
Manual
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls
Submitted by oualidpro, mau. Selected submission by: oualidpro.
https://github.com/Cyfrin/2023-07-escrow/tree/main//src/EscrowFactory.sol#L7
Imports could be organized more systematically
The contract used interfaces should be imported first, followed by all other files. The examples below do not follow this layout.
File: /src/EscrowFactory.sol
7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
Informational
Submitted by oualidpro, codeslide, mau. Selected submission by: oualidpro.
https://github.com/Cyfrin/2023-07-escrow/tree/main/src/EscrowFactory.sol#L72
Constants should be defined rather than using magic numbers
Even assembly can benefit from using readable constants instead of hex/numeric literals
File: /src/EscrowFactory.sol
//@audit Try to make a `constant` with `0xff` value
72: bytes1(0xff),
Informational
Manual
Submitted by ABA.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L45-L47
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L28-L31
Project code style uses a short, and somehwat asymetrical, form of comment header separation. To improve readability, change headers to a more visible version.
Example this header :
/////////////////////
// View functions
/////////////////////
can be rewritten using https://github.com/transmissions11/headers as:
/*//////////////////////////////////////////////////////////////
VIEW FUNCTIONS
//////////////////////////////////////////////////////////////*/
Also, align text to after the @dev tag in comments for better visibility:
Example, align:
/// @dev Sets the Escrow transaction values for `price`, `tokenContract`, `buyer`, `seller`, `arbiter`, `arbiterFee`. All of
/// these values are immutable: they can only be set once during construction and reflect essential deal terms.
/// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have
/// been sent to this address.
as
/// @dev Sets the Escrow transaction values for `price`, `tokenContract`, `buyer`, `seller`, `arbiter`, `arbiterFee`. All of
/// these values are immutable: they can only be set once during construction and reflect essential deal terms.
/// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have
/// been sent to this address.
Submitted by pontifex, Bbash, JohnLaw, 97Sabit, Udsen. Selected submission by: pontifex.
The word chosing should be choosing. There are two instances in the scope:
src/Escrow.sol
19: /// Therefore, careful consideration should be taken when chosing the token.
src/EscrowFactory.sol
19: /// Therefore, careful consideration should be taken when chosing the token.
Submitted by iepathos, devtooligan, 0xSCSamurai. Selected submission by: 0xSCSamurai.
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
In the extremely unlikely scenario where the total funds that arrived in the Escrow contract's address when transferred by the buyer, is LESS than what the buyer sent, i.e. tokenContract.balanceOf(address(this)) < price, for whatever reason, then the above if statement will revert and no Escrow contract will be created, as intended.
All good, but see my recommendation.
n/a
Frustration.
VSC, manual.
However, maybe just my noobness but it's my suggestion to have some solution implemented that would eliminate this highly unlikely potential risk of reverting and temporarily DoS-ing buyer's attempt to create escrow contract, by ensuring that the price amount of tokens the buyer transfers to the Escrow contract's address is exactly what will arrive there, always.
Additionally, which tokens/token contracts may contribute to this potential "risk" of causing less tokens to arrive than what was sent?
Submitted by Omsify.
Unnecessary comparison leading to waste of gas
In EscrowFactory::newEscrow(), in lines 48-50 the code performs a comparsion between the deployed Escrow address and precomputed Escrow address. The thing is, the addresses will never differ. This can be checked with a fuzz-test. This issue leads to both higher EscrowFactory contract deployment cost and EscrowFactory::newEscrow() function interaction cost. POC (foundry fuzz test):
// EscrowFactoryComparsion.t.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;
import {Test, console} from "forge-std/Test.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {EscrowFactory} from "src/EscrowFactory.sol";
contract EscrowFactoryComparsion is Test {
ERC20Mock public immutable mockErc20;
EscrowFactory public immutable ef;
constructor() {
mockErc20 = new ERC20Mock();
ef = new EscrowFactory();
}
function testAddressCalculation(address buyer,
uint256 price,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt) public {
vm.assume(buyer!=address(0));
vm.assume(seller!=address(0));
vm.assume(arbiterFee<price);
mockErc20.mint(buyer, price);
vm.prank(buyer);
mockErc20.approve(address(ef), price);
vm.prank(buyer);
ef.newEscrow(price, mockErc20, seller, arbiter, arbiterFee, salt);
}
}
And in the foundry.toml file:
[fuzz]
runs = 50_000
fail_on_revert = true
I tested this with 50000 fuzz runs.
About 34 wasted gas units on EscrowFactory::newEscrow() interaction.
Foundry, fuzz-tests
Remove the unnecessary comparsion (lines 48-50 in EscrowFactory.sol)
Submitted by TorpedopistolIxc41.
Address collusion may occur with create2 with low probability.
In case newescrow function fails cause of address collusion user lose huge amount of gas . if we check the codesize before deployment and see if any contract deployment exist with the computed address we can revert with a low gas loss. In the test case which is testSameSaltReverts in EscrowFactory test suit in case same salt used can lead to same address so in revert phase with customized code-block ; loss of gas minimal.Down below you can see the difference of gas in test suits
//EscrowFactoryTest:testSameSaltReverts() (gas: 8937393460516713538) --> before implementation!
//EscrowFactoryTest:testSameSaltReverts() (gas: 765957) --> after implementation!
uint codesz;
assembly {
codesz := extcodesize(computedAddress)
}
if(codesz !=0) revert EscrowFactory__ContractAlreadyDeployed();
code update down below
function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {
address computedAddress = computeEscrowAddress(
type(Escrow).creationCode,
address(this),
uint256(salt),
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
uint codesz;
assembly {
codesz := extcodesize(computedAddress)
}
if(codesz !=0) revert EscrowFactory__ContractAlreadyDeployed();
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
if (address(escrow) != computedAddress) {
revert EscrowFactory__AddressesDiffer();
}
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;
}
Foundry-test suits
Submitted by 0xCiphky, 1nc0gn170. Selected submission by: 1nc0gn170.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L32-L51
It is better to choose an arbiter who is neither the seller nor the buyer. If the arbiter acts as any one of them, then the integrity becomes questionable. Ensuring that the arbiter is another actor who can impartially judge both sides will give more integrity to the protocol."
constructor(
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) {
if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();
+ if (buyer == arbiter || seller == arbiter) revert Escrow__InvalidArbiter();
if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
...
}
Natspec comments for smart contracts, interfaces and libraries.Submitted by alymurtazamemon.
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrow.sol#L6
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/IEscrowFactory.sol#L7
It is recommended by solidity docs that solidity contracts should be fully annotated using NatSpec for all public interfaces (everything in the ABI).
Add the Natspec comments in the above-mentioned contracts.
Submitted by hasanza.
EscrowFactory.computeEscrowAddress() computes the contract address. Refactoring the computation process using Assembly results in considerable gas savings.
The function EscrowFactory.computeAddress() can be refactored to calculate addresses using Assembly to save gas on deployment and upon each call.
Gas
Forge, Foundry Toolkit (gas report, gas snapshots)
Refactor address computation using Yul. Consider the refactored EscrowFactory contract below:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;
import {IEscrowFactory} from "./IEscrowFactory.sol";
import {IEscrow} from "./IEscrow.sol";
import {Escrow} from "./Escrow.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {Create2} from "@openzeppelin/contracts/utils/Create2.sol";
/// @author Cyfrin
/// @title EscrowFactory
/// @notice Factory contract for deploying Escrow contracts.
contract EscrowFactory is IEscrowFactory {
using SafeERC20 for IERC20;
/// @inheritdoc IEscrowFactory
/// @dev msg.sender must approve the token contract to spend the price amount before calling this function.
/// @dev There is a risk that if a malicious token is used, the dispute process could be manipulated.
/// Therefore, careful consideration should be taken when chosing the token.
function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {
// Compute
bytes32 bytecodeHash = keccak256(abi.encodePacked(type(Escrow).creationCode, abi.encode(price, tokenContract, msg.sender, seller, arbiter, arbiterFee)));
address computedAddress = computeAddress(salt, bytecodeHash, address(this));
// Transfer funds
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);
// Ensure address equality
if (address(escrow) != computedAddress) {
revert EscrowFactory__AddressesDiffer();
}
// Return escrow instance
emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
return escrow;
}
/**
* @dev Returns the address where a contract will be stored if deployed via {deploy} from a contract located at
* `deployer`. If `deployer` is this contract's address, returns the same value as {computeAddress}.
*/
function computeAddress(bytes32 salt, bytes32 bytecodeHash, address deployer) public pure returns (address addr) {
/// @solidity memory-safe-assembly
assembly {
// Load free mem ptr on stack
let ptr := mload(0x40)
// Store bytecodeHash at 0x00 (ptr) + 0x40, so starting at 64thnd byte
mstore(add(ptr, 0x40), bytecodeHash)
// Store salt at ptr + 0x20, so starting at 32nd byte
mstore(add(ptr, 0x20), salt)
// Store deployer addr starting at 0x00. Since it 20 bytes and rightalined, 12 garbage bytes on the left
mstore(ptr, deployer)
// We want to hash addr+salt+byteCodeHash, so move the starting ptr to the 11th byte
let start := add(ptr, 0x0b)
// Set garbage byte right before the first addr byte (the 12th byte from the start) to 0xff
// This is to avoid clash with CREATE
//from 0x0b(11th byte), store 0xff, which gets stored at 12th byte
mstore8(start, 0xff)
// Hash from the 12th byte until the 85th byte (1byte0xff+20byteAddr+32byteSalt+32byteBytecodeHash)
addr := keccak256(start, 85)
}
}
}
With this change in place, considerable gas savings were observed.
An overall gas saving of 23261 gas was seen across all tests. The tests testComputedAddressEqualsDeployedAddress() and testCreatingEscrowEmitsEvent() saw the highest gas savings of 8377 gas and 9344 gas respectively. All other tests saw a saving of around 200 gas.
The contract size reduced by 249 bytes, from 8622 to 8373. The deployment cost went from 1720616 to1670760, seeing a gas saving of 49856 gas.
Note that the tests have to be modified slightly to account for this change. The changes shown in the modified test below should be replicated in all tests where Escrow is instantiated:
function testCreatingEscrowEmitsEvent() public hasTokensApprovedForSending {
bytes32 bytecodeHash = keccak256(abi.encodePacked(type(Escrow).creationCode, abi.encode(PRICE, i_tokenContract, BUYER, SELLER, ARBITER, ARBITER_FEE)));
address computedAddress = escrowFactory.computeAddress(SALT1, bytecodeHash, address(escrowFactory));
vm.prank(BUYER);
vm.expectEmit(true, true, true, true, address(escrowFactory));
emit EscrowCreated(computedAddress, BUYER, SELLER, ARBITER);
escrowFactory.newEscrow(PRICE, i_tokenContract, SELLER, ARBITER, ARBITER_FEE, SALT1);
}
Submitted by hasanza.
The seller may want to receive the funds to a different address than the one used at Escrow creation.
Seller/ Auditor may want to receive funds to a different address than he is using as his identity for the Escrow. There can be various reasons for this, seller may have lost access to the wallet after entereing the Escrow or wants to keeps his identity wallet separate from fund wallet. As such, seller should be allowed to change address or ad an extra recipient address.
Low
Manual Review
If buyer agrees, then allow seller to change address or add an extra recipient address. As an example, a function allowing seller to reset the seller address may be used to achieve this.
Submitted by serialcoder.
The EscrowFactory.computeEscrowAddress() can return an incorrect predicted address to an external caller.
The EscrowFactory.computeEscrowAddress() is defined as a public function, allowing both an external caller and the EscrowFactory contract itself (i.e., the newEscrow()) to execute it.
In case of an external call, if a caller inputs the deployer parameter incorrectly, the computeEscrowAddress() will return an incorrect predicted address since the inputted deployer parameter will be used for computing the predicted address.
function computeEscrowAddress(
bytes memory byteCode,
@> address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {
address predictedAddress = address(
uint160(
uint256(
keccak256(
abi.encodePacked(
bytes1(0xff),
@> deployer,
salt,
keccak256(
abi.encodePacked(
byteCode, abi.encode(price, tokenContract, buyer, seller, arbiter, arbiterFee)
)
)
)
)
)
)
);
return predictedAddress;
}
The EscrowFactory.computeEscrowAddress() can return an incorrect predicted address if an external caller inputs the deployer parameter incorrectly.
Manual Review
I recommend hard-coding the deployer parameter with the address(this) instead, as shown below.
This will guarantee that no matter whether an external caller or the EscrowFactory contract itself (i.e., the newEscrow()) will execute the computeEscrowAddress(), the function will return a correct predicted address.
function computeEscrowAddress(
bytes memory byteCode,
- address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {
address predictedAddress = address(
uint160(
uint256(
keccak256(
abi.encodePacked(
bytes1(0xff),
- deployer,
+ address(this),
salt,
keccak256(
abi.encodePacked(
byteCode, abi.encode(price, tokenContract, buyer, seller, arbiter, arbiterFee)
)
)
)
)
)
)
);
return predictedAddress;
}