Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

[H] Possible transfer of ownership to an incorrect account, leading to loss of admin access

fbi

Summary

This vulnerability is in the Adminable.solcontract, which is in charge of access control and transfer of ownership. It is possible that the initial admin transfers the ownership of the contract to an incorrect account. In that case, the team will lose access to the priviledged functions of Sablier Stream.

Vulnerability Details

This is the only function in the Adminable.solcontract:

function transferAdmin(address newAdmin) public virtual override onlyAdmin {
// Effect: update the admin.
admin = newAdmin;
​
// Log the transfer of the admin.
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
}

newAdmin is intended to be the address of the new owner. Now, there will be a problem when the current owner put inputs an unintended address as newAdmin.

If this is done, the unintended entity will be the new admin, and the team will lose access to it!

I am aware the first waiver to this might be, "this is due to human error." Yes, because in reality, human errors are bound to happen.

The best thing to do from a security PoV is to check against human errors so the code can be safe. OpenZeppelin had to create Ownable2Step because of this.

Impact

These are the impact of this vulnerability:

  • the team will lose control of the contracts that use Adminable.soland not be able to call their privileged functions

  • the unintended entity can exploit the Stream at will!

PoC

  • Imagine the control of the contract is intended to be transferred to address(0x1234)

  • But access is unintentionally sent to address(0x12345)

  • address(0x12345)becomes the new owner

  • address(0x12345)can exploit the contracts inheriting Adminable.sol

Coded PoC

Create a file and name it admin.t.soland paste this:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;
​
import { Test } from "forge-std/src/Test.sol";
import "../src/abstracts/Adminable.sol";
import "../src/libraries/Errors.sol";
​
// Instantiating contract to aid test
contract TestAdminable is Adminable {
constructor(address _admin) {
admin = _admin;
}
}
​
contract AdminableTest is Test {
Adminable adminable;
address intendedEntity = address(0x1234);
address wrongPerson = address(0x12345);
​
function setUp() public {
adminable = new TestAdminable(initialAdmin);
}
​
function testTransferToWrongEntity() public {
vm.prank(initialAdmin);
adminable.transferAdmin(address(0x12345));
​
assertEq(adminable.admin(), wrongPerson); // the wrong person has
}
​
}
​
​
​

When you run this test, this should show on CLI; meaning it passed.

Ran 1 test for tests/admin.t.sol:AdminableTest
[PASS] testTransferToWrongEntity() (gas: 20501)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.93ms (560.40µs CPU time)

Adminable.solis an abstract contract, so I wrote TestAdminableas a contract to instantiate it.

The intended entity to be the new owner is address(0x1234)but the initial admin erroneously made address(0x12345) the new admin.

Now, the wrongPersonis in charge of the contract!

Tools Used

Foundry

Recommendations

The OZ team created Ownable2Step for this reason.

Use this library or implement it locally in Adminable.sol. Then this vulnerability can be blocked.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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