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

The owner of swan contract can access some special buyer restricted functions

bhunter

Summary

The owner of swan contract is also an operator and any operators can access any functions with onlyAuthorized modifier in buyerAgent contract. This will leave to an open door for updating state of buyer agent without consent of owner of buyerAgent.

Vulnerability Details

In contets details it is expressed swan owner and operators are trusted.

  • Swan Owner (Trusted): This is the wallet that deploys Swan by default.

  • Swan Operator (Trusted): For every Buyer, there is an onlyAuthorized modifier that ensures the modified function is callable by BuyerAgent owner, or an address such that swan.isOperator(addr) is true. These operators simply exist so that buyer owner's dont have to be online all the time to call purchase, updateState etc., and can instead let the Swan operators call it for them. The operators are currently centralized, and belong to FirstBatch.

That migh make sense for functions like purchase, updateState due to the mentioned reason above, however not for withdraw(), which is defined as:

/// @notice Function to withdraw the tokens from the contract.
/// @param _amount amount to withdraw.
/// @dev If the current phase is `Withdraw` buyer can withdraw any amount of tokens.
/// @dev If the current phase is not `Withdraw` buyer has to leave at least `minFundAmount` in the contract.
function withdraw(uint96 _amount) public onlyAuthorized {
(, Phase phase,) = getRoundPhase();
​
// if we are not in Withdraw phase, we must leave
// at least minFundAmount in the contract
if (phase != Phase.Withdraw) {
// instead of checking `treasury - _amount < minFoundAmount`
// we check this way to prevent underflows
if (treasury() < minFundAmount() + _amount) {
revert MinFundSubceeded(_amount);
}
}
​
// transfer the tokens to the owner of Buyer
swan.token().transfer(owner(), _amount);
}

ı believe this function supposed to be called by the owner. Both the comment in the function and the following test in BuyerAgent.test.ts support what ı said.

it("should NOT allow non-owner to withdraw", async function () {
// only owner can withdraw in withdraw phase
// try to withdraw by user
await expect(buyerAgent.connect(user).withdraw(ROYALTY_FEE))
.to.be.revertedWithCustomError(buyerAgent, "Unauthorized")
.withArgs(user.address);
});

if we look at the withdraw method, onlyAuthorized modifier is used, which defined as:

modifier onlyAuthorized() {
// if its not an operator, and is not an owner, it is unauthorized
if (!swan.isOperator(msg.sender) && msg.sender != owner()) {
revert Unauthorized(msg.sender);
}
_;
}

it can be seen an operator can call any function with the modifier.For operators this can make sense but the owner of swan is also set as an operator:

function initialize(
SwanMarketParameters calldata _marketParameters,
LLMOracleTaskParameters calldata _oracleParameters,
// contracts
address _coordinator,
address _token,
address _buyerAgentFactory,
address _swanAssetFactory
) public initializer {
//SNIP
​
// swan is an operator
isOperator[address(this)] = true;
// owner is an operator
isOperator[msg.sender] = true;
}

Even if the owner of swan contract is trusted, it should not interfere with state of buyerAgent without the consent of users in this case.

Impact

Affecting buyerAgent state without its owner's consent, leading to undesired outcomes

Tools Used

None

Recommendations

I believe wrong modifier is used here. Instead of onlyAuthorized , onlyOwner modifier should be used.

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.