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

Incorrect token return mechanism in LLMOracleRegistry's Unregister Function

zuby

Summary

The LLMOracleRegistry contract uses token.approve() instead of token.transfer() when returning staked tokens to users during unregistration. This requires users to perform an additional transaction to retrieve their tokens and can lead to accumulated allowances, making the process inefficient and potentially risky.

Vulnerability Details

The vulnerability exists in the unregister function of the LLMOracleRegistry contract:
https://github.com/Cyfrin/2024-10-swan-dria/blob/c8686b199daadcef3161980022e12b66a5304f8e/contracts/llm/LLMOracleRegistry.sol#L117-L131

// File: contracts/llm/LLMOracleRegistry.sol#L117-L131
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);
}

The issue lies in line 130 where token.approve() is used instead of token.transfer(). This creates a two-step process where users must:
https://github.com/Cyfrin/2024-10-swan-dria/blob/c8686b199daadcef3161980022e12b66a5304f8e/contracts/llm/LLMOracleRegistry.sol#L130

  1. Call unregister() to get approval

  2. Call transferFrom() in a separate transaction to actually retrieve their tokens

Impact

Increased Gas Costs: Users must execute two transactions instead of one
Poor User Experience: Additional complexity in token retrieval process
Security Risk: Tokens remain in the contract longer than necessary
Allowance Accumulation: Multiple unregistrations can lead to accumulated allowances

POC Test and Results:

it("POC: demonstrates issue with unregister using approve instead of transfer", async function () {
// First register as both generator and validator
await token.connect(oracle).approve(oracleRegistryAddress, GENERATOR_STAKE + VALIDATOR_STAKE);
await oracleRegistry.connect(oracle).register(OracleKind.Generator);
await oracleRegistry.connect(oracle).register(OracleKind.Validator);
​
// Check initial balances
const registryBalance = await token.balanceOf(oracleRegistryAddress);
const oracleBalance = await token.balanceOf(oracle.address);
expect(registryBalance).to.equal(GENERATOR_STAKE + VALIDATOR_STAKE);
​
// Unregister as generator
await oracleRegistry.connect(oracle).unregister(OracleKind.Generator);
​
// Prove tokens are still in registry after unregister
const registryBalanceAfterUnregister = await token.balanceOf(oracleRegistryAddress);
expect(registryBalanceAfterUnregister).to.equal(registryBalance);
​
// Demonstrate allowance behavior
const allowanceBeforeSecondUnregister = await token.allowance(oracleRegistryAddress, oracle.address);
await oracleRegistry.connect(oracle).unregister(OracleKind.Validator);
const finalAllowance = await token.allowance(oracleRegistryAddress, oracle.address);
console.log("Initial allowance:", allowanceBeforeSecondUnregister.toString());
console.log("Final allowance:", finalAllowance.toString());
console.log("VALIDATOR_STAKE:", VALIDATOR_STAKE.toString());
});

Test Results:

Initial allowance: 0
Final allowance: 100000000000000000
VALIDATOR_STAKE: 100000000000000000

Tools Used

  • Manual Review

  • Hardhat Testing Framework

  • Ethereum Development Environment

Recommendations

Replace the current implementation with direct token transfer:

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);
​
// transfer tokens directly to user
token.transfer(msg.sender, amount);
}
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.