Dria
Swan
NFTHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Unchecked `transferFrom` call in `register` function leaving the contract in an inconsistent state and potentially registering the user without a successful stake transfer in `LLMOracleRegistry.sol`

safdie

Summary

The LLMOracleRegistry contract’s register function calls transferFrom to transfer the staking amount from the user to the contract but does not verify whether the transfer succeeded. If transferFrom fails (due to insufficient balance, lack of approval, or other issues), the function will not revert, leaving the contract in an inconsistent state and potentially registering the user without a successful stake transfer.

Vulnerability Details

In the LLMOracleRegistry contract, the register function stakes a specified amount by calling token.transferFrom(msg.sender, address(this), amount). However, the result of this call is not checked. This could lead to unintended registration of users who have not successfully transferred their stake, as the function proceeds even if transferFrom fails.

The vulnerable code is shown below:

function register(LLMOracleKind kind) public {
uint256 amount = getStakeAmount(kind);
​
// ensure the user is not already registered
if (isRegistered(msg.sender, kind)) {
revert AlreadyRegistered(msg.sender);
}
​
// ensure the user has enough allowance to stake
if (token.allowance(msg.sender, address(this)) < amount) {
revert InsufficientFunds();
}
​
// Transfer stake from user to the contract (unchecked)
token.transferFrom(msg.sender, address(this), amount);
​
// register the user
registrations[msg.sender][kind] = amount;
emit Registered(msg.sender, kind);
}

To demonstrate this vulnerability, simulate a scenario where transferFrom fails (e.g., insufficient balance or lack of allowance) and observe that register still proceeds without reverting.

const { expect } = require("chai");
const { ethers } = require("hardhat");
​
describe("LLMOracleRegistry - Unchecked transferFrom", function () {
let registry, token;
let owner, oracle;
const stakeAmount = ethers.utils.parseEther("10");
​
beforeEach(async function () {
[owner, oracle] = await ethers.getSigners();
​
// Deploy ERC20 token contract
const ERC20 = await ethers.getContractFactory("ERC20Mock");
token = await ERC20.deploy("Test Token", "TTK", owner.address, ethers.utils.parseEther("1000"));
await token.deployed();
​
// Deploy LLMOracleRegistry contract
const Registry = await ethers.getContractFactory("LLMOracleRegistry");
registry = await upgrades.deployProxy(Registry, [stakeAmount, stakeAmount, token.address], { initializer: "initialize" });
await registry.deployed();
​
// Transfer some tokens to oracle but less than required stake
await token.transfer(oracle.address, ethers.utils.parseEther("5"));
});
​
it("should not register oracle if transferFrom fails", async function () {
// Approve contract to spend less than stake amount
await token.connect(oracle).approve(registry.address, ethers.utils.parseEther("5"));
​
// Attempt to register with insufficient balance
await expect(registry.connect(oracle).register(0)).to.be.revertedWith("Transfer failed");
​
// Check that registration did not occur
expect(await registry.isRegistered(oracle.address, 0)).to.equal(false);
});
});

The test demonstrates that attempting to register without sufficient balance or allowance should revert, as transferFrom does not succeed. In this case, the original code would have incorrectly registered the oracle even though the stake transfer did not complete, but with the fix (shown below), it will correctly revert.

Impact

If the transferFrom call fails, users will be incorrectly registered without their stake being successfully transferred to the contract. This could lead to several issues:

  • Users may incorrectly believe they have staked and are registered as oracles.

  • The contract's state could be inconsistent with the actual funds held.

  • Unregistered users could gain access to oracle functions, potentially undermining the protocol’s integrity.

Tools Used

Manual review.

Recommendations

To resolve this issue, modify the register function to check the return value of transferFrom and revert if the transfer fails.

function register(LLMOracleKind kind) public {
uint256 amount = getStakeAmount(kind);
​
// ensure the user is not already registered
if (isRegistered(msg.sender, kind)) {
revert AlreadyRegistered(msg.sender);
}
​
// ensure the user has enough allowance to stake
if (token.allowance(msg.sender, address(this)) < amount) {
revert InsufficientFunds();
}
​
// Transfer stake from user to the contract and check for success
bool success = token.transferFrom(msg.sender, address(this), amount);
require(success, "Transfer failed");
​
// register the user
registrations[msg.sender][kind] = amount;
emit Registered(msg.sender, kind);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[KNOWN] - Low-35 Unsafe use of transfer()/transferFrom() with IERC20

Support

FAQs

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