[Audit Report] VTVL

0xDave·2022년 11월 25일
TILblockchaincode4renaethereum
0

Ethereum

목록 보기
63/112

9월 20일 컨테스트 시작
11월 1일 리포트 발표
High Risk Findings (2)
Medium Risk Findings (10)
Low Risk and Non-Critical Issues (20)


Scope


내가 생각했던 결함들

단순히 리포트를 읽는 것보다 내가 먼저 코드 리뷰를 해보고 어떤 부분이 결함이 있을 것 같은지 생각한 후에 리포트를 읽는 것이 훨씬 많은 도움이 될 것이다. 아래는 내가 리포트를 읽기 전 생각했던 결함들이다.

1. setAdmin

//AccessProtected.sol

    function setAdmin(address admin, bool isEnabled) public onlyAdmin {
        require(admin != address(0), "INVALID_ADDRESS");
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }

Admin이 다른 주소를 Admin으로 지정할 수 있는 함수다. 그런데 잘 생각해보면 다음에 지정된 주소가 Admin이 되자마자 원래 Admin 주소의 권한을 박탈할 수 있다. 발생할 확률이 조금 낮지만 그래도 위험하다고 생각한다. 단지 setAdmin을 호출해서 나머지 모든 Admin에게 false를 부여하면 모든 자금이 탈취 당할 것이고 더 이상 되돌릴 수 없다.. 현재 내가 생각하는 가장 바람직한 방법은 Admin 체계를 두 개로 나누는 것 정도? 명확한 해답은 리포트를 봐야 할 것 같다.


2. createClaim

//VTVLVesting.sol

    function createClaim(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) external onlyAdmin {
        _createClaimUnchecked(_recipient, _startTimestamp, _endTimestamp, _cliffReleaseTimestamp, _releaseIntervalSecs, _linearVestAmount, _cliffAmount);
    }

클레임 설정에 직접적으로 관여하는 함수다. _startTimestamp를 현재 block.timestamp로 하는 것이 좀 더 안전하지 않을까? _createClaimUnchecked에서는 단지 0보다 크면 조건을 충족하는 걸로 되어있다.


3. _createClaimUnchecked

//VTVLVesting.sol

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount,
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {
        require(_recipient != address(0), "INVALID_ADDRESS");
        require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT");
        //...
    }

두 번째 require문에서 계산식의 우선순위가 명확하지 않다.(나만 그렇게 생각할 수도..) 괄호를 하나 더 넣어주는 것이 깔끔하지 않을까?


4. createClaimsBatch

//VTVLVesting.sol

function createClaimsBatch(
        address[] memory _recipients, 
        uint40[] memory _startTimestamps, 
        uint40[] memory _endTimestamps, 
        uint40[] memory _cliffReleaseTimestamps, 
        uint40[] memory _releaseIntervalsSecs, 
        uint112[] memory _linearVestAmounts, 
        uint112[] memory _cliffAmounts) 
        external onlyAdmin {
        
        
        uint256 length = _recipients.length;
        require(_startTimestamps.length == length &&
                _endTimestamps.length == length &&
                _cliffReleaseTimestamps.length == length &&
                _releaseIntervalsSecs.length == length &&
                _linearVestAmounts.length == length &&
                _cliffAmounts.length == length, 
                "ARRAY_LENGTH_MISMATCH"
        );

        for (uint256 i = 0; i < length; i++) {
            _createClaimUnchecked(_recipients[i], _startTimestamps[i], _endTimestamps[i], _cliffReleaseTimestamps[i], _releaseIntervalsSecs[i], _linearVestAmounts[i], _cliffAmounts[i]);
        }

        // No need for separate emit, since createClaim will emit for each claim (and this function is merely a convenience/gas-saver for multiple claims creation)
    }

반복문을 볼 때마다 드는 생각인데 array의 갯수가 너무 많을 때 revert가 되는 것을 방지해야 하지 않나 싶다. 각 array.lengh의 limit 값을 정해야 할 것 같은데 이게 또 모든 array 반복문에 적용되는 건지 궁금하다.

몇 개 더 있긴 한데 애매해서 리포트를 보고 판단하는 것이 나을 것 같다.


새로 배운 것 - msgSender()

보안성을 고려한 tx.origin이라고 할 수 있다. msg.sender를 리턴해주며, Openzeppelin의 openzeppelin/contracts/utils/Context.sol에서 찾아볼 수 있다.


리포트

[H-01] LOSS OF VESTED AMOUNTS

////VTVLVesting.sol

    function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        // Fetch the claim
        Claim storage _claim = claims[_recipient];
        // Calculate what the claim should finally vest to
        uint112 finalVestAmt = finalVestedAmount(_recipient);

        // No point in revoking something that has been fully consumed
        // so require that there be unconsumed amount
        require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");

        // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn
        uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

        // Deactivate the claim, and release the appropriate amount of tokens
        _claim.isActive = false;    // This effectively reduces the liability by amountRemaining, so we can reduce the liability numTokensReservedForVesting by that much
        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

        // Tell everyone a claim has been revoked.
        emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
    }

클레임을 revoke 할 수 있는 함수다. 핵심은 아래 문장이다. 현재 vesting이 남아있는 금액에서 출금한 금액을 뺀 것 모두 revoke한다. 여기서 문제되는 것은 뭘까?


uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

리포트를 읽기 전에 코드를 봤을 때 문제점을 발견하지 못했다. user 입장에서 생각하지 못했기 때문이다. 만약에 user가 vesting이 다 된 토큰을 출금도 안 했는데 admin이 revoke한다면? 말 그대로 출금 안 한 토큰은 더 이상 건들일 수 없고 손실처리 된다. user 입장에서는 머릿속에 물음표가 가득할 것이다. 출금도 안 했는데 감히 revoke를 해? 내 돈 내놔!


[M-01] SUPPLY CAP OF VARIABLESUPPLYERC20TOKEN IS NOT PROPERLY ENFORCED

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        // If we're using maxSupply, we need to make sure we respect it
        // mintableSupply = 0 means mint at will
        if(mintableSupply > 0) {
            require(amount <= mintableSupply, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount;
        }
        _mint(account, amount);
    }

mintableSupply는 맨 처음 constructor에서 0 이상 값으로 설정된다. 계속 민팅을 하다가 Supply 값 만큼 민팅을 하면 결국엔 mintableSupply는 0이 될 수 밖에 없다. 그 다음엔 민팅이 안 되는게 맞다. 그런데 위 코드를 보면 mintableSupply가 0일 경우, if문을 무시하고 민팅을 하게된다. 이 다음부터는 무한민팅이 가능해진다. 심사단은 해당 문제를 High가 아니라 Medium이라고 판단했다. onlyAdmin modifier가 들어가서 위 문제가 발생하기엔 특정 조건이 필요하다고 봤기 때문이다.

내가 이 문제를 발견 못 한 이유는 Supply 수를 넘어서 민팅할 수 있는 경우를 생각 못 했기 때문이다. 당장에 민팅하는 그 순간만 생각했지 모두 민팅되고 나서 그 이후를 생각하지 않았다. 항상 코드를 볼 때 전체적인 사이클을 생각하도록 하자. 추가적으로 onlyAdmin이 붙어도 문제가 발생할 소지가 있으면 일단 리포트에 적도록 하자.


[M-03] POSSIBLE DOS ON VESTINGRECIPIENTS DUE TO LACK OF DISPOSAL MECHANISM

    // Track the recipients of the vesting
    address[] internal vestingRecipients;
	
    function allVestingRecipients() external view returns (address[] memory) {
        return vestingRecipients;
    }

	//...
    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {

		//...
        // Done with checks

        // Effects limited to lines below
        claims[_recipient] = _claim; // store the claim
        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
        vestingRecipients.push(_recipient); // add the vesting recipient to the list
        emit ClaimCreated(_recipient, _claim); // let everyone know
    }

    function createClaim(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) external onlyAdmin {
        _createClaimUnchecked(_recipient, _startTimestamp, _endTimestamp, _cliffReleaseTimestamp, _releaseIntervalSecs, _linearVestAmount, _cliffAmount);
    }

vestingRecipients는 베스팅 받는 주소가 기록되는 array다. createClaim이 호출되면 vestingRecipients.push(_recipient);를 통해 주소가 array에 추가된다. 여기까지는 문제가 없어보인다. 그런데 다른 코드를 살펴보면 vestingRecipients에서 주소를 제거하는 코드가 없다. 만약 제거하는 단계 없이 계속 추가만 된다면 쌓이고 쌓여서 나중에는 allVestingRecipients()가 실행되지 않을 것이다. 크기가 정해져 있지 않고 계속 추가되는 ever-growing array를 호출하면 block gas limit을 초과해서 해당 데이터를 불러올 수 없기 때문이다. judge의 코멘트에 따르면 이론적으로 배열은 2**256 -1 만큼의 주소를 저장할 수 있지만 배열을 다 채우기도 전에 함수 호출이 실패할 것이라고 한다. 따라서 추천하는 방법은 withdraw 함수가 호출될 때 해당 배열에서 주소를 제거하는 코드를 추가하는 것이다.

처음에 이러한 이슈가 있다는 것을 보고 뒤통수를 망치로 맞은 느낌이었다.

나는 왜 이 생각을 못 했을까? 한편으로는 레포트를 읽는 것이 재밌다는 느낌까지 있었다. 내가 생각하지 못 했던 것들을 생각할 수 있게 해주고, 시야를 더 넓혀주는 느낌이었다. 내가 이 문제를 떠올리지 못 했던 이유는 단순하다. 경험이 부족하기 때문이다. 여러 프로젝트를 경험해보고 다른 레포트를 읽어나가다 보면 충분히 생각할 수 있을 것이다.


[M-04] NOT ABLE TO CREATE CLAIM

    modifier hasNoClaim(address _recipient) {
        Claim storage _claim = claims[_recipient];
        require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
        _;
    }

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {
    }
	
	//...

    function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        // Fetch the claim
        Claim storage _claim = claims[_recipient];
        // Calculate what the claim should finally vest to
        uint112 finalVestAmt = finalVestedAmount(_recipient);

        // No point in revoking something that has been fully consumed
        // so require that there be unconsumed amount
        require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");

        // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn
        uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

        // Deactivate the claim, and release the appropriate amount of tokens
        _claim.isActive = false;    // This effectively reduces the liability by amountRemaining, so we can reduce the liability numTokensReservedForVesting by that much
        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

        // Tell everyone a claim has been revoked.
        emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
    }

_createClaimUnchecked을 실행할 때 hasNoClaim 제약을 받는다. 즉, _claim.startTimestamp == 0이어야 하는데 이것만 봐서는 문제가 없다. 그런데 다음과 같은 시나리오를 한 번 생각해보자.

  1. Admin A가 B를 위해 Claim을 만든다.
  2. B의 Claim에서 문제를 발견한 A는 B의 Claim을 revoke 한다.
  3. 문제가 잘 해결돼서 다시 B의 Claim을 만들어준다.

위 시나리오는 충분히 있을 수 있는 시나리오다. A가 Claim을 만들 때 정보를 잘못 입력했거나, B가 착각해서 revoke를 요청했거나 등등 여러 경우가 있을 수 있다. 그런데 A는 다시 Claim을 만들 수 없을 것이다. _claim.startTimestamp == 0이어야 하는데 이미 이전에 Claim을 만들었기 때문에 _claim.startTimestamp의 값은 0이 아니다. 따라서 다른 주소를 설정해야 새로운 Claim을 생성할 수 있다. 이를 해결할 수 있는 방법은 revokeClaim 함수에 _claim.startTimestamp == 0을 추가하는 것이다.


나는 왜 이 시나리오를 생각하지 못 했을까? 코드를 보면서 그 때의 생각의 흐름을 다시 살펴보면 revokeClaim 함수를 볼 때 '아 revoke 기능이 있구나' 정도로만 생각했지 'revoke를 하고 다시 Claim을 만들 때 문제는 없나?' 까지는 생각하지 못 했다. 앞으로 코드를 읽을 때 단순히 해당 코드만 보지 말고 다른 함수와 연달아서 동작했을 때 또는 해당 함수와 관련 있는 함수에 영향을 주는 부분은 어떤 부분이 있는지 살펴보도록 하자.


[M-07] VESTING SCHEDULE START AND END TIME CAN BE SET IN THE PAST

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {

        require(_recipient != address(0), "INVALID_ADDRESS");
        require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
        require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
	//...
    }

위 문제는 내가 레포트를 읽기 전에 생각했던 문제랑 비슷하다. require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");이 단지 0보다 크게 되어있기 때문에 현재 timestamp 값을 정확히 가져올 수 없고, 과거의 timestamp를 입력할 수 있기 때문이다. 해당 결함을 얘기한 리서처는 _endTimestamp도 과거로 설정하면 클레임을 성정한 사람의 의도와 다르게 이미 베스팅이 완료돼서 토큰 수령자가 모든 토큰을 클레임해서 가져갈 수 있다고 작성했다.

여기서 재밌는건 judge에서 실제 사례를 들어 예외를 얘기한 것. 파운더가 직원들 보상해주려고 TGE 전에 베스팅을 시작하는 경우가 종종 있기 때문에 완전한 결함이라고 할 수는 없다는 주장이다. 이건 약간 특이 케이스인 것 같다는 생각과 동시에 컨트랙트에 모든 케이스를 고려하는 것이 굉장히 어렵다는 것을 알게 됐다. 내가 생각지도 못 한 케이스로 원래 의도와 다르게 동작할 수도 있구나..


[M-10] REENTRANCY MAY ALLOW AN ADMIN TO STEAL FUNDS

    function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    
        // Allow the owner to withdraw any balance not currently tied up in contracts.
        uint256 amountRemaining = tokenAddress.balanceOf(address(this)) - numTokensReservedForVesting;

        require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");

        // Actually withdraw the tokens
        // Reentrancy note - this operation doesn't touch any of the internal vars, simply transfers
        // Also following Checks-effects-interactions pattern
        tokenAddress.safeTransfer(_msgSender(), _amountRequested);

        // Let the withdrawal known to everyone
        emit AdminWithdrawn(_msgSender(), _amountRequested);
    }

위 함수는 Reentrancy 공격이 가능하다. 물론 Admin 자격이 있을 때만. 함수를 재호출해도 numTokensReservedForVesting 값이 바뀌지 않기 때문에 amountRemaining >= _amountRequested 조건을 만족하는 한 계속 자금을 탈취할 수 있다. 항상 출금과 관련된 함수에는 nonReentrant가 있는지 체크하자. 추가적으로 위 이슈는 Admin이 해커라는 전제가 깔려있다. Admin을 무조건적으로 신뢰하기 보다 해커가 단순히 user일 것이라는 생각을 버려야 한다.


Low Risk [01]

The setAdmin() function in AccessProtected.sol can be used to revoke all admins. This could be a feature to completely renounce ownership of the contract after all claims are set or could be a bug in which one admin either intentionally or unintentionally removes all admin (or all other admins except himself).

내가 생각했던 결함은 Low Risk에 있었다. 크지 않은 비중이지만 그래도 뭔가 뿌듯한 느낌이다. 호호


Feedback

  1. 컨트랙트를 사용하는 user의 입장에서 생각할 것.
  2. 전체적인 사이클을 생각할 것. 처음부터 끝까지 정상적인 시나리오대로 프로젝트가 진행됐을 때 또는 그 이후에 문제가 발생할만한 요소가 없는지 체크하자.
  3. 현재의 경험에 집중하되 계속 레포트를 읽어나가서 경험을 쌓을 것. 계속 읽고 고민하면서 시야를 넓힐 것.
  4. 하나의 함수를 볼 때, 그냥 그 함수만 보지 말고 관련 있는 다른 함수에 어떤 부분에 영향을 주는지 잘 살펴볼 것.
  5. 항상 출금과 관련된 함수에는 nonReentrant가 있는지 체크.
  6. 해커가 단순히 user일 것이라는 생각을 버려야 한다.

출처 및 참고자료

  1. VTVL contest - Findings & Analysis Report
  2. ERC 20 주요 업데이트 사항(0.6 버젼) override, virtual, _beforeTokenTransfer의 역할
profile
0xDave
Just BUIDL :)
이전 포스트

[Ethereum] 아비트라지를 위한 여정 12편 - ABI

다음 포스트

[Ethereum] 아비트라지를 위한 여정 13편 - OOP & Factory

0개의 댓글