Fjord Token Staking
Fjord
DeFiFoundry
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Tokens Locked Due to Unchecked `transfer` Failure in `FjordAuction::auctionEnd`

andir16

Description

The FjordAuction::auctionEnd function attempts to transfer the auctioned tokens to the owner if no bids are placed. However, the contract does not check the return value of the transfer function. If the transfer fails (for example, due to a malicious or non-standard ERC20 token), the contract incorrectly assumes the transfer was successful, marking the auction as ended. This results in the auctioned tokens being locked in the contract permanently, as no further actions can be taken to retrieve them.

Impact

If the transfer function fails and its return value is not checked, the auctioned tokens will be stuck in the contract. The owner will not receive the tokens, and since the auction is marked as ended, there is no way to recover or redistribute the tokens. This could lead to a significant loss, especially if a large amount of tokens is involved.

Proof of Concepts

A test was conducted using a mock ERC20 token (MaliciousERC20Mock) that overrides the transfer function to always return false. The test demonstrated that:

  • The FjordAuction::auctionEnd function did not revert when the transfer failed.

  • The auction was marked as ended, but the tokens remained in the contract.

  • The owner did not receive the auctioned tokens, confirming that they were effectively locked in the contract.

Malicious Token Contract

Here is the code for the MaliciousERC20Mock contract that was used in the test:

// SPDX-License-Identifier: MIT
pragma solidity =0.8.21;
​
import "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
​
contract MaliciousERC20Mock is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
​
// Override transfer to always return false
function transfer(address recipient, uint256 amount) public override returns (bool) {
return false;
}
}

Test Case

Here is the relevant test code:

// SPDX-License-Identifier: MIT
pragma solidity =0.8.21;
​
import "forge-std/Test.sol";
import "src/FjordAuction.sol";
import "../mocks/MaliciousERC20Mock.sol";
import {ERC20BurnableMock} from "../mocks/ERC20BurnableMock.sol";
​
contract TestAuctionWithMaliciousToken is Test {
FjordAuction public auction;
ERC20BurnableMock public fjordPoints;
MaliciousERC20Mock public maliciousToken;
uint256 public biddingTime = 1 weeks;
uint256 public totalTokens = 1000 ether;
​
function setUp() public {
fjordPoints = new ERC20BurnableMock("FjordPoints", "fjoPTS");
maliciousToken = new MaliciousERC20Mock("MaliciousToken", "MAL");
auction = new FjordAuction(
address(fjordPoints),
address(maliciousToken),
biddingTime,
totalTokens
);
​
deal(address(maliciousToken), address(auction), totalTokens);
}
​
function testAuctionEndWithNoBiddersAndFailingTransfer() public {
skip(biddingTime);
auction.auctionEnd();
​
// Ensure the auction has ended
assertEq(auction.ended(), true);
​
// Check that the tokens are still in the contract because the transfer failed
assertEq(maliciousToken.balanceOf(address(auction)), totalTokens);
​
// Ensure that the owner did not receive the tokens
assertEq(maliciousToken.balanceOf(auction.owner()), 0);
}
}

Recommended Mitigation

To prevent this issue, the FjordAuction::auctionEnd function should check the return value of the transfer function. If the transfer fails, the transaction should be reverted to ensure that the auctioned tokens are not locked in the contract.

The project already uses SafeTransferLib from the Solmate library in the FjordStaking contract. It is recommended to apply the same approach in the FjordAuction contract to handle token transfers safely.

Here’s how you can modify the FjordAuction::auctionEnd function to use SafeTransferLib:

if (totalBids == 0) {
- auctionToken.transfer(owner, totalTokens);
+ auctionToken.safeTransfer(owner, totalTokens);
return;
}

This mitigation ensures that if the transfer fails, the contract reverts the transaction, preventing the auction tokens from being locked in the contract. By using SafeTransferLib, you align with existing practices in the codebase and improve the overall security and reliability of the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.