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

Lack of withdrawal аunctionality in `unregister` function could mislead users into thinking they are refunded when, in reality, they are not in `LLMOracleRegistry.sol`

safdie

Summary

The LLMOracleRegistry contract’s unregister function lacks proper withdrawal functionality for user stakes when unregistering as an oracle. The function currently approves the staked amount back to the user but does not actually transfer the funds. This could mislead users into thinking they are refunded when, in reality, they are not. Without a direct withdrawal mechanism, users would need to explicitly call transferFrom using the allowance, which many may not realize. This vulnerability affects user trust and usability, potentially locking funds in the contract.

Vulnerability Details

In the provided LLMOracleRegistry contract, the unregister function is intended to refund the user’s stake upon unregistering as an oracle. However, rather than transferring the staked amount back to the user, it only approves the allowance for the user, requiring them to initiate a subsequent transferFrom to withdraw their tokens.

Below is the code snippet that highlights the vulnerability in unregister:

function unregister(LLMOracleKind kind) public returns (uint256 amount) {
amount = registrations[msg.sender][kind];
​
// ensure the user is registered
if (amount == 0) {
revert NotRegistered(msg.sender);
}
​
// unregister the user
delete registrations[msg.sender][kind];
emit Unregistered(msg.sender, kind);
​
// approve its stake back
token.approve(msg.sender, token.allowance(address(this), msg.sender) + amount);
}

To demonstrate the vulnerability, a user must call register with sufficient staking allowance and then call unregister. After unregistering, they would still need to execute a transferFrom to retrieve the approved tokens. Users unaware of this requirement may believe their funds were automatically returned.

Let`s check it with a test:

const { expect } = require("chai");
const { ethers } = require("hardhat");
​
describe("LLMOracleRegistry", 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();
​
// Fund oracle and approve staking allowance
await token.transfer(oracle.address, stakeAmount);
await token.connect(oracle).approve(registry.address, stakeAmount);
});
​
it("should approve stake on unregister but not transfer", async function () {
// Register as an oracle
await registry.connect(oracle).register(0);
​
// Unregister and verify funds are only approved
await registry.connect(oracle).unregister(0);
​
// Check that the contract still holds the stake
expect(await token.balanceOf(registry.address)).to.equal(stakeAmount);
​
// Check the allowance for oracle is set to the stake amount
expect(await token.allowance(registry.address, oracle.address)).to.equal(stakeAmount);
​
// User must manually withdraw funds using transferFrom
await expect(token.connect(oracle).transferFrom(registry.address, oracle.address, stakeAmount))
.to.emit(token, "Transfer")
.withArgs(registry.address, oracle.address, stakeAmount);
});
});

The test demonstrates that after calling unregister, the tokens remain in the contract. The oracle must make an additional transferFrom call to retrieve their funds, which users may overlook, thinking the funds were automatically returned.

Impact

Users who call unregister will not immediately receive their staked funds, which could lead to confusion or funds being "stuck" if they are unaware of the additional transferFrom requirement. This flaw impacts user experience and trust in the contract's functionality, as it deviates from standard staking/unstaking processes where funds are automatically refunded.

Tools Used

Manual review.

Recommendations

To resolve this, modify the unregister function to use transfer instead of approve to ensure that funds are returned directly to the user upon registering.

function unregister(LLMOracleKind kind) public returns (uint256 amount) {
amount = registrations[msg.sender][kind];
​
// ensure the user is registered
if (amount == 0) {
revert NotRegistered(msg.sender);
}
​
// unregister the user
delete registrations[msg.sender][kind];
emit Unregistered(msg.sender, kind);
​
// directly transfer stake back
require(token.transfer(msg.sender, amount), "Transfer failed");
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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