Summary
There's no check if the buyer and the arbiter are the same address
Vulnerability Details
In Escrow contract : the role of the arbiter is to solve the dispute if the seller or the buyer initiated it.
So if the buyer is the arbiter as well; then the buyer can initiate a dispute, then resolving it while setting a buyerReward input equals to i_tokenContract.balanceOf(address(this)) - i_arbiterFee , so he will get the whole tokens balance (as a buyerReward + arbiterFee) and the seller will get nothing.
The same risk might be encountered if the arbiter address is set equal to the seller address; but since the buyer is the one who will deploy the escrow; the probability of this scenario occurring is very low.
Impact
Malicious buyer can steal the full rewards while the seller gets nothing.
Proof of Concept
Line 32-51:
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;
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;
i_arbiterFee = arbiterFee;
}
testBuyerAndArbiterAreTheSame test is set in the EscrowTest.t.sol file to test the behavior of the escrow if the arbiter and the buyer are set the same (the basic arguments are copied from testDeployEscrowFromFactory test and updated).
add this test to the EscrowTest.t.sol file :
function testBuyerAndArbiterAreTheSame() public {
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,
BUYER,
ARBITER_FEE,
SALT1
);
vm.stopPrank();
assertEq(escrow.getPrice(), PRICE);
assertEq(address(escrow.getTokenContract()), address(i_tokenContract));
assertEq(escrow.getBuyer(), BUYER);
assertEq(escrow.getSeller(), SELLER);
assertEq(escrow.getArbiter(), BUYER);
assertEq(escrow.getArbiterFee(), ARBITER_FEE);
vm.prank(BUYER);
escrow.initiateDispute();
assertEq(uint256(escrow.getState()), uint256(IEscrow.State.Disputed));
uint256 buyerAwards = i_tokenContract.balanceOf(address(escrow)) -
ARBITER_FEE;
vm.startPrank(BUYER);
escrow.resolveDispute(buyerAwards);
assertEq(uint256(escrow.getState()), uint256(IEscrow.State.Resolved));
assertEq(i_tokenContract.balanceOf(address(escrow)), 0);
assertEq(i_tokenContract.balanceOf(BUYER), PRICE);
assertEq(i_tokenContract.balanceOf(SELLER), 0);
}
Test result:
$ forge test --match-test testBuyerAndArbiterAreTheSame
Running 1 test for test/unit/EscrowTest.t.sol:EscrowTest
[PASS] testBuyerAndArbiterAreTheSame() (gas: 915836)
Test result: ok. 1 passed; 0 failed; finished in 2.79ms
Tools Used
Manual Testing.
Recommendations
Update the constructor in the Escrow contract to check if buyer == arbiter & if seller == arbiter:
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();
+ if (buyer == arbiter) revert();
+ if (seller == arbiter) revert();
i_price = price;
i_tokenContract = tokenContract;
i_buyer = buyer;
i_seller = seller;
i_arbiter = arbiter;
i_arbiterFee = arbiterFee;
}
