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

Unprotected transferAdmin Function Leading to Admin Key Compromise

zibrasismail

Summary

The transferAdmin function in the Adminable contract lacks validation against zero address transfers. This oversight allows the admin role to be permanently lost by transferring it to address(0), effectively bricking all admin-protected functionality in the contract.

Check code snippet

https://github.com/Cyfrin/2024-10-sablier/blob/main/src/abstracts/Adminable.sol#L33-L41

Vulnerability Details

The vulnerability exists in the Adminable contract's transferAdmin function:

function transferAdmin(address newAdmin) public virtual override onlyAdmin {
admin = newAdmin; // Vulnerable: No zero-address check
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
}

The function allows setting admin to any address, including address(0), without validation. Once set to the zero address:
No address can satisfy the onlyAdmin modifier check
The admin role becomes permanently inaccessible

Impact

Permanent Loss of Admin Control - Admin functions become permanently inaccessible, No way to transfer admin role to a new address

Impact

The following Forge test demonstrates the vulnerability:

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22;
​
import { Test } from "forge-std/src/Test.sol";
import "../../src/abstracts/Adminable.sol";
import "../../src/interfaces/IAdminable.sol";
import "../../src/libraries/Errors.sol";
​
contract MockProtocol is Adminable {
uint256 public value;
bool public isPaused;
​
constructor() {
admin = msg.sender;
}
​
function updateValue(uint256 newValue) external onlyAdmin {
value = newValue;
}
​
function pause() external onlyAdmin {
isPaused = true;
}
}
​
contract AdminCompromiseTest is Test {
MockProtocol public protocol;
address public initialAdmin;
​
function setUp() public {
initialAdmin = address(this);
protocol = new MockProtocol();
}
​
function test_AdminCompromiseByZeroAddress() public {
// 1. Transfer admin to zero address
protocol.transferAdmin(address(0));
assertEq(protocol.admin(), address(0));
// 2. Demonstrate that previous admin can no longer access admin functions
vm.expectRevert(abi.encodeWithSelector(
Errors.CallerNotAdmin.selector,
address(0),
initialAdmin
));
protocol.updateValue(100);
// 3. Verify critical functions are permanently locked
vm.startPrank(initialAdmin);
vm.expectRevert(abi.encodeWithSelector(
Errors.CallerNotAdmin.selector,
address(0),
initialAdmin
));
protocol.pause();
vm.stopPrank();
}
}
forge test --match-contract AdminCompromiseTest -vv
Compiler run successful!
​
Ran 1 test for tests/audit/AdminCompromiseTest.t.sol:AdminCompromiseTest
[PASS] test_AdminCompromiseByZeroAddress() (gas: 21732)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 695.80µs (205.70µs CPU time)
​
Ran 1 test suite in 6.09ms (695.80µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual code review, VS Code

Recommendations

Add Zero Address Validation

Implement Two-Step Transfer Process ( if possible )

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
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.

Give us feedback!