Summary
In the FjordAuction constructor, contract owner is incorrectly set to the msg.sender which is the instance of the FjordAuctionFactory. When auction ends without any bids assets are returned to the owner. However, FjordAuctionFactory does not have capability to manage these assets and they will remain locked.
Vulnerability Details
FjordAuctionFactory#L59
FjordAuction.sol#L134
In the FjordAuctionFactory::createAuction function, the constructor from the FjordAuction contract is called:
function createAuction(
address auctionToken,
uint256 biddingTime,
uint256 totalTokens,
bytes32 salt
) external onlyOwner {
@> address auctionAddress = address(
@> new FjordAuction{ salt: salt }(fjordPoints, auctionToken, biddingTime, totalTokens)
);
IERC20(auctionToken).transferFrom(msg.sender, auctionAddress, totalTokens);
emit AuctionCreated(auctionAddress);
}
The mentioned constructor looks like this:
constructor(
address _fjordPoints,
address _auctionToken,
uint256 _biddingTime,
uint256 _totalTokens
) {
if (_fjordPoints == address(0)) {
revert InvalidFjordPointsAddress();
}
if (_auctionToken == address(0)) {
revert InvalidAuctionTokenAddress();
}
fjordPoints = ERC20Burnable(_fjordPoints);
auctionToken = IERC20(_auctionToken);
@> owner = msg.sender;
auctionEndTime = block.timestamp.add(_biddingTime);
totalTokens = _totalTokens;
}
As we can see, the owner of the auction is set to msg.sender. In the case where the FjordAuctionFactory contract calls the constructor of the FjordAuction, msg.sender will be the FjordAuctionFactory contract, not the caller of the FjordAuctionFactory::createAuction function.
PoC
Make a new test file auctionFactory.t.sol in test/unit/:
pragma solidity =0.8.21;
import "forge-std/Test.sol";
import "src/FjordAuctionFactory.sol";
import { ERC20BurnableMock } from "../mocks/ERC20BurnableMock.sol";
contract TestAuction is Test {
using SafeMath for uint256;
AuctionFactory public factory;
ERC20BurnableMock public fjordPoints;
ERC20BurnableMock public auctionToken;
address public owner = address(0x1);
uint256 public biddingTime = 1 weeks;
uint256 public totalTokens = 1000 ether;
function setUp() public {
fjordPoints = new ERC20BurnableMock("FjordPoints", "fjoPTS");
auctionToken = new ERC20BurnableMock("AuctionToken", "AUCT");
vm.prank(owner);
factory= new AuctionFactory(address(fjordPoints));
deal(address(auctionToken), owner, totalTokens);
}
function testAuctionOwner() public {
bytes32 salt=0x7465737400000000000000000000000000000000000000000000000000000000;
vm.startPrank(owner);
auctionToken.approve(address(factory), totalTokens);
factory.createAuction(
address(auctionToken),
biddingTime,
totalTokens,
salt
);
vm.stopPrank();
FjordAuction auction = FjordAuction(0xD04e7b550AbafB44d41b4B36db060f03aB3D4766);
address auctionOwner = auction.owner();
assert(auctionOwner != owner);
assert(auctionOwner == address(factory));
console.log("Auction owner:", auctionOwner);
console.log("It should be: ", owner);
console.log("But it is: ", address(factory));
vm.warp(vm.getBlockTimestamp() + biddingTime + 1);
auction.auctionEnd();
assert(0 == auctionToken.balanceOf(owner));
assert(totalTokens == auctionToken.balanceOf(address(factory)));
}
}
Impact
The owner of the auction will not be the one who called the FjordAuctionFactory::createAuction function and sent the tokens for the auction, but rather the FjordAuctionFactory contract itself. When creating new auction, creator sends the tokens to the created contract:
function createAuction(address auctionToken, uint256 biddingTime, uint256 totalTokens,bytes32 salt
) external onlyOwner {
(...)
@> IERC20(auctionToken).transferFrom(msg.sender, auctionAddress, totalTokens);
(...)
}
In the case when the auction ends without any bids, the tokens will be sent to the the auction owner i.e. FjordAuctionFactory address, not to the token owner, and they will remain locked in the mentioned contract:
function auctionEnd() external {
if (block.timestamp < auctionEndTime) {
revert AuctionNotYetEnded();
}
if (ended) {
revert AuctionEndAlreadyCalled();
}
ended = true;
emit AuctionEnded(totalBids, totalTokens);
if (totalBids == 0) {
@> auctionToken.transfer(owner, totalTokens);
return;
}
multiplier = totalTokens.mul(PRECISION_18).div(totalBids);
uint256 pointsToBurn = fjordPoints.balanceOf(address(this));
fjordPoints.burn(pointsToBurn);
}
Tools Used
Manual code review / Foundry tests
Recommendations
Modify the constructor in the FjordAuction contract by adding an additional parameter that will represent the address of the auction owner:
constructor(
address _fjordPoints,
address _auctionToken,
uint256 _biddingTime,
uint256 _totalTokens,
+ address auctionOwner
) {
if (_fjordPoints == address(0)) {
revert InvalidFjordPointsAddress();
}
if (_auctionToken == address(0)) {
revert InvalidAuctionTokenAddress();
}
fjordPoints = ERC20Burnable(_fjordPoints);
auctionToken = IERC20(_auctionToken);
- owner = msg.sender;
+ owner = auctionOwner;
auctionEndTime = block.timestamp.add(_biddingTime);
totalTokens = _totalTokens;
}
