Summary
The FjordAuction contract is vulnerable to silent failures in token transfers due to unchecked return values from transfer and transferFrom calls. This oversight can lead to incorrect bid recordings and potential loss of funds, as the contract assumes transfers are successful without verifying their outcomes.
L151
fjordPoints.transferFrom(msg.sender, address(this), amount);
L174
fjordPoints.transfer(msg.sender, amount);
Vulnerability Details
1.Deploy Contract:
2.bid function:
Call the bid function with a number of tokens.
The custom ERC20 contract will return false on transferFrom, but since there is no return value check, the FjordAuction contract will assume the transfer was successful.
As a result, the contract will record a bid without actually receiving any tokens.
3.unbid function:
Call the unbid function to withdraw the bid.
The custom ERC20 contract will return false on transfer, but the FjordAuction contract will assume the transfer was successful.
As a result, the user does not receive their tokens back, but the contract will reduce the number of recorded bids, causing data inconsistencies and potential loss of funds.
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/FjordAuction.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
contract FjordAuctionTest is Test {
FjordAuction public auction;
CustomERC20 public fjordPoints;
CustomERC20 public auctionToken;
address public user = address(0x1);
function setUp() public {
fjordPoints = new CustomERC20("FjordPoints", "FJP");
auctionToken = new CustomERC20("AuctionToken", "AUC");
auction = new FjordAuction(address(fjordPoints), address(auctionToken), 3600, 1000 ether);
fjordPoints.mint(user, 100 ether);
auctionToken.mint(address(auction), 1000 ether);
}
function testUncheckedERC20Transfer() public {
vm.startPrank(user);
fjordPoints.approve(address(auction), 10 ether);
auction.bid(10 ether);
assertEq(auction.bids(user), 10 ether);
assertEq(fjordPoints.balanceOf(user), 100 ether);
auction.unbid(10 ether);
assertEq(auction.bids(user), 0);
assertEq(fjordPoints.balanceOf(user), 100 ether);
vm.stopPrank();
}
}
contract CustomERC20 is ERC20, ERC20Burnable {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
function transferFrom(address, address, uint256) public pure override returns (bool) {
return false;
}
function transfer(address, uint256) public pure override returns (bool) {
return false;
}
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
forge test --match-path test/FjordAuctionTest.t.sol -vvvv
[⠊] Compiling...
[⠃] Compiling 1 files with Solc 0.8.21
[⠒] Solc 0.8.21 finished in 1.81s
Compiler run successful!
Ran 1 test for test/FjordAuctionTest.t.sol:FjordAuctionTest
[PASS] testUncheckedERC20Transfer() (gas: 79089)
Traces:
[104127] FjordAuctionTest::testUncheckedERC20Transfer()
├─ [0] VM::startPrank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [24652] CustomERC20::approve(FjordAuction: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 10000000000000000000 [1e19])
│ ├─ emit Approval(owner: ECRecover: [0x0000000000000000000000000000000000000001], spender: FjordAuction: [0xF62849F9A0B5Bf2913
b396098F7c7019b51A820a], value: 10000000000000000000 [1e19]) │ └─ ← [Return] true
├─ [51496] FjordAuction::bid(10000000000000000000 [1e19])
│ ├─ [513] CustomERC20::transferFrom(ECRecover: [0x0000000000000000000000000000000000000001], FjordAuction: [0xF62849F9A0B5Bf29
13b396098F7c7019b51A820a], 10000000000000000000 [1e19]) │ │ └─ ← [Return] false
│ ├─ emit BidAdded(bidder: ECRecover: [0x0000000000000000000000000000000000000001], amount: 10000000000000000000 [1e19])
│ └─ ← [Stop]
├─ [517] FjordAuction::bids(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 10000000000000000000 [1e19]
├─ [0] VM::assertEq(10000000000000000000 [1e19], 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Return]
├─ [2585] CustomERC20::balanceOf(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 100000000000000000000 [1e20]
├─ [0] VM::assertEq(100000000000000000000 [1e20], 100000000000000000000 [1e20]) [staticcall]
│ └─ ← [Return]
├─ [3802] FjordAuction::unbid(10000000000000000000 [1e19])
│ ├─ [453] CustomERC20::transfer(ECRecover: [0x0000000000000000000000000000000000000001], 10000000000000000000 [1e19])
│ │ └─ ← [Return] false
│ ├─ emit BidWithdrawn(bidder: ECRecover: [0x0000000000000000000000000000000000000001], amount: 10000000000000000000 [1e19])
│ └─ ← [Stop]
├─ [517] FjordAuction::bids(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [585] CustomERC20::balanceOf(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
│ └─ ← [Return] 100000000000000000000 [1e20]
├─ [0] VM::assertEq(100000000000000000000 [1e20], 100000000000000000000 [1e20]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.02ms (1.04ms CPU time)
Ran 1 test suite in 17.18ms (3.02ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
The contract records bids even when the token transfer fails, leading to a mismatch between recorded bids and actual token holdings.
Users may believe their bids are valid when no tokens have been transferred, and they may not receive tokens back when unbidding.
Tools Used
Recommendations
Use require statements to ensure that transfer and transferFrom calls succeed.
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);
+ require(fjordPoints.transferFrom(msg.sender, address(this), amount), "Transfer failed");
emit BidAdded(msg.sender, amount);
}
function unbid(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
uint256 userBids = bids[msg.sender];
if (userBids == 0) {
revert NoBidsToWithdraw();
}
if (amount > userBids) {
revert InvalidUnbidAmount();
}
bids[msg.sender] = bids[msg.sender].sub(amount);
totalBids = totalBids.sub(amount);
- fjordPoints.transfer(msg.sender, amount);
+ require(fjordPoints.transfer(msg.sender, amount), "Transfer failed");
emit BidWithdrawn(msg.sender, amount);
}
