CodeHawks Escrow Contract - Competition Details
Cyfrin
Foundry
40,000 USDC
View results
Previous Next
Submission Details
Severity: gas

`resolveDispute` can save gas by not making another call to the `i_tokenContract` balance

stermi

Summary

resolveDispute can avoid making another call to i_tokenContract.balanceOf to save gas to calculate the amount of token to be received by the seller account.

Vulnerability Details

The total amount of token that will be distributed to the actors (buyer, seller, arbiter depending on the fees and buyerAward) is given by the amount of tokens owned by the Escrow contract.

Given i_arbiterFee and buyerAward and given that uint256 tokenBalance = i_tokenContract.balanceOf(address(this)); this will be the actual distribution

  • Token received by arbiter: i_arbiterFee

  • Token received by buyer: buyerAward

  • Token received by seller: tokenBalance - i_arbiterFee - buyerAward

There is no reason to make an external call again to the i_tokenContract contract (external call cost + SLOAD) because we can already calculate the amount of tokens that will be received by the seller

Impact

// SPDX-License-Identifier: MIT
​
pragma solidity 0.8.18;
​
import {Test} from "forge-std/Test.sol";
import {IEscrow, Escrow} from "../../src/Escrow.sol";
import {EscrowFactory} from "../../src/EscrowFactory.sol";
import {EscrowTestBase} from "../EscrowTestBase.t.sol";
import {DeployEscrowFactory} from "../../script/DeployEscrowFactory.s.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
​
contract SEscrowTest is Test, EscrowTestBase {
EscrowFactory public escrowFactory;
IEscrow public escrow;
uint256 public buyerAward = 0;
​
​
function setUp() external {
DeployEscrowFactory deployer = new DeployEscrowFactory();
escrowFactory = deployer.run();
}
​
modifier escrowDeployed() {
vm.startPrank(BUYER);
ERC20Mock(address(i_tokenContract)).mint(BUYER, PRICE);
ERC20Mock(address(i_tokenContract)).approve(address(escrowFactory), PRICE);
escrow = escrowFactory.newEscrow(PRICE, i_tokenContract, SELLER, ARBITER, ARBITER_FEE, SALT1);
vm.stopPrank();
_;
}
​
function testDispute() public escrowDeployed {
uint256 buyerStartingBalance = ERC20Mock(address(i_tokenContract)).balanceOf(BUYER);
uint256 sellerStartingBalance = ERC20Mock(address(i_tokenContract)).balanceOf(SELLER);
uint256 arbiterStartingBalance = ERC20Mock(address(i_tokenContract)).balanceOf(ARBITER);
​
buyerAward = 1e16;
vm.prank(BUYER);
escrow.initiateDispute();
​
vm.prank(ARBITER);
escrow.resolveDispute(buyerAward);
​
uint256 expectedSellarReward = PRICE - buyerAward - ARBITER_FEE;
​
assertEq(ERC20Mock(address(i_tokenContract)).balanceOf(address(escrow)), 0);
assertEq(ERC20Mock(address(i_tokenContract)).balanceOf(BUYER), buyerStartingBalance + buyerAward);
assertEq(ERC20Mock(address(i_tokenContract)).balanceOf(SELLER), sellerStartingBalance + expectedSellarReward);
assertEq(ERC20Mock(address(i_tokenContract)).balanceOf(ARBITER), arbiterStartingBalance + ARBITER_FEE);
}
}

By testing with forge snapshot we can see that the gas saving is -15177 (-1.779%)

Tools Used

Manual

Recommendations

The client should consider making the following modification to the resolveDispute function

function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
// other code...
​
- tokenBalance = i_tokenContract.balanceOf(address(this));
- if (tokenBalance > 0) {
- i_tokenContract.safeTransfer(i_seller, tokenBalance);
- }
+ uint256 sellerAward = tokenBalance - totalFee;
+ if (tokenBalance - totalFee > 0) {
+ i_tokenContract.safeTransfer(i_seller, sellerAward);
+ }
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.