Summary
In FjordAuction.sol https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L143
* @notice Places a bid in the auction.
* @param amount The amount of FjordPoints to bid.
*/
function bid(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
bids[msg.sender] = bids[msg.sender].add(amount);
totalBids = totalBids.add(amount);
fjordPoints.transferFrom(msg.sender, address(this), amount);
emit BidAdded(msg.sender, amount);
}
the function bid does not check if the balance is equal or greater than amount. "In this case, the transferFrom operation will fail if the user does not have a sufficient FjordPoints balance, and an error will be thrown. According to the ERC20 standard, the transferFrom function will fail and revert in the following cases:
If the sender (msg.sender) does not have a sufficient balance.
If the sender has not granted sufficient spending allowance to the contract.
Therefore, the call to fjordPoints.transferFrom(msg.sender, address(this), amount); will automatically revert the transaction and throw an error if the user does not have a sufficient balance or spending allowance.
In this scenario, even without an explicit balance check, the transaction will fail in case of insufficient balance. However, this approach has a few disadvantages:
The error message will not be customized, and it may not provide the user with clear information about why the transaction failed.
It may result in unnecessary gas consumption because the transaction will progress until the transferFrom call and only fail at that point.
A better approach would be to check the user's balance at the beginning of the transaction.
Vulnerability Details
Without balance check, there will be more gas consumption
To compare this check, we need to write a test.
First add this function to FjordAuction.sol
function bidWithCheck(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
if (fjordPoints.balanceOf(msg.sender) < amount) {
revert InsufficientBalance();
}
bids[msg.sender] = bids[msg.sender].add(amount);
totalBids = totalBids.add(amount);
fjordPoints.transferFrom(msg.sender, address(this), amount);
emit BidAdded(msg.sender, amount);
}
also add this error to FjordAuction.sol
error InsufficientBalance();
now add this function to FjordPoints.sol
function mintTest(address to, uint256 amount) external {
_mint(to, amount);
}
now create a test file named FjordAuction.t.sol and add this code
pragma solidity ^0.8.21;
import "forge-std/Test.sol";
import "../src/FjordAuction.sol";
import "../src/FjordPoints.sol";
contract GasTest is Test {
FjordAuction auction;
FjordPoints fjordPoints;
address user = address(0x1);
uint256 auctionDuration = 7 days;
uint256 totalTokens = 1000 ether;
function setUp() public {
fjordPoints = new FjordPoints();
auction = new FjordAuction(
address(fjordPoints),
address(0x2),
auctionDuration,
totalTokens
);
fjordPoints.mintTest(user, 100 ether);
}
function testGasUsageWithoutCheck() public {
vm.startPrank(user);
fjordPoints.approve(address(auction), 1000 ether);
uint256 gasStart = gasleft();
try auction.bid(1000 ether) {
} catch {
}
uint256 gasUsed = gasStart - gasleft();
console.log("Gas used without balance check:", gasUsed);
vm.stopPrank();
}
function testGasUsageWithCheck() public {
vm.startPrank(user);
fjordPoints.approve(address(auction), 1000 ether);
uint256 gasStart = gasleft();
try auction.bidWithCheck(1000 ether) {
} catch {
}
uint256 gasUsed = gasStart - gasleft();
console.log("Gas used with balance check:", gasUsed);
vm.stopPrank();
}
}
now, when you start both tests testGasUsageWithCheck and testGasUsageWithoutCheck you will see the difference.
Here you can see testGasUsageWithoutCheck:
Traces:
[98431] GasTest::testGasUsageWithoutCheck()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← [Return]
├─ [24652] FjordPoints::approve(FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000000 [1e21])
│ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000001, spender: FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] true
├─ [54889] FjordAuction::bid(1000000000000000000000 [1e21])
│ ├─ [5470] FjordPoints::transferFrom(0x0000000000000000000000000000000000000001, FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000000 [1e21])
│ │ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000001, spender: FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 0)
│ │ └─ ← [Revert] revert: ERC20: transfer amount exceeds balance
│ └─ ← [Revert] revert: ERC20: transfer amount exceeds balance
├─ [0] console::log("Gas used without balance check:", 57867 [5.786e4]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
now lets see testGasUsageWithCheck:
Traces:
[51089] GasTest::testGasUsageWithCheck()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← [Return]
├─ [24652] FjordPoints::approve(FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000000 [1e21])
│ ├─ emit Approval(owner: 0x0000000000000000000000000000000000000001, spender: FjordAuction: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] true
├─ [7503] FjordAuction::bidWithCheck(1000000000000000000000 [1e21])
│ ├─ [2608] FjordPoints::balanceOf(0x0000000000000000000000000000000000000001) [staticcall]
│ │ └─ ← [Return] 100000000000000000000 [1e20]
│ └─ ← [Revert] InsufficientBalance()
├─ [0] console::log("Gas used with balance check:", 10481 [1.048e4]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
When running the test testGasUsageWithoutCheck the usage is 57867 but while running with testGasUsageWithCheck the usage is 10481.
Impact
To calculate the current gas fees, we can follow these steps:
1. Gas Units
a) Gas used without a balance check: 57,867
b) Gas used with a balance check: 10,481
2. Gas Price
The gas price varies depending on network conditions and is usually expressed in gwei. For example, 1 gwei = 0.000000001 ETH. Let's assume the average gas price is 20 gwei (though this may change).
3. ETH Price
To determine the USD equivalent of Ethereum, we need to use the current market price. Let's assume the current ETH/USD price is 1700 USD.
4. Calculation
First, we calculate the total gas cost in ETH.
Then, we convert this amount to USD.
Let's calculate it.
In terms of gas fees:
A transaction without a balance check costs approximately 1.97 USD.
A transaction with a balance check costs approximately 0.36 USD.
This means that, without a balance check, the user pays approximately 1.61 USD more. This shows that you can significantly reduce costs by avoiding
unnecessary gas consumption.
Tools Used
Manual review
Recommendations
A better approach would be to check the user's balance at the beginning of the transaction.
function bid(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
+ if (fjordPoints.balanceOf(msg.sender) < amount) {
+ revert InsufficientBalance();
+ }
bids[msg.sender] = bids[msg.sender].add(amount);
totalBids = totalBids.add(amount);
fjordPoints.transferFrom(msg.sender, address(this), amount);
emit BidAdded(msg.sender, amount);
}
This way, the user is provided with a clearer error message, and unnecessary gas consumption is avoided.
