[Audit Report] FEI and TRIBE Redemption

0xDave·2022년 11월 28일

Ethereum

목록 보기

66/112

Scope

PSM

PSM은 1:1 비율로 바로 교환할 수 있도록 해주는 시스템을 말한다. 다오메이커에서 운영하는 USDC PSM의 경우, 사용자는 USDC와 DAI를 1:1로 교환 가능하다. 이번 레포트 scope에 포함되어 있는 SimpleFeiDaiPSM은 Fei와 Dai를 1:1로 교환할 수 있게 해주는 컨트랙트다.

내가 생각했던 결함들

1. 하나의 주소를 재사용하지 못 함

//RariMerkleRedeemer.sol

    function _claim(
        address _cToken,
        uint256 _amount,
        bytes32[] calldata _merkleProof
    ) internal virtual {
        // check: verify that claimableAmount is zero, revert if not
        require(claims[msg.sender][_cToken] == 0, "User has already claimed for this cToken.");

        // check: verify cToken and amount and msg.sender against merkle root
        bytes32 leafHash = keccak256(abi.encodePacked(msg.sender, _amount));
        require(MerkleProof.verifyCalldata(_merkleProof, merkleRoots[_cToken], leafHash), "Merkle proof not valid.");

        // effect: update claimableAmount for the user
        claims[msg.sender][_cToken] = _amount;

        emit Claimed(msg.sender, _cToken, _amount);
    }

    function _redeem(address cToken, uint256 cTokenAmount) internal virtual {
        // check: amount must be greater than 0
        require(cTokenAmount != 0, "Invalid amount");

        // check: verify that the user's claimedAmount+amount of this cToken doesn't exceed claimableAmount for this cToken
        require(
            redemptions[msg.sender][cToken] + cTokenAmount <= claims[msg.sender][cToken],
            "Amount exceeds available remaining claim."
        );

        // effect: increment the user's claimedAmount
        redemptions[msg.sender][cToken] += cTokenAmount;

        uint256 baseTokenAmountReceived = previewRedeem(cToken, cTokenAmount);

        // interaction: safeTransferFrom the user "amount" of "cToken" to this contract
        IERC20(cToken).safeTransferFrom(msg.sender, address(this), cTokenAmount);
        IERC20(baseToken).safeTransfer(msg.sender, baseTokenAmountReceived);

        emit Redeemed(msg.sender, cToken, cTokenAmount, baseTokenAmountReceived);
    }

팀의 의도가 맞는지 다시 한 번 봐야겠지만, 일단 코드만 살펴보면 하나의 주소 당 claim을 한 번 밖에 못 한다. 내가 생각하는 시나리오는 다음과 같다.

Alice가 claim을 한다.
Alice가 redeem 할 수 있는 모든 토큰을 redeem을 한다.
어떤 사유로 Alice가 토큰을 받을 일이 있어서(팀에서 추가적으로 토큰을 claim할 수 있게 해준다던가) 추가로 claim과 redeem을 하려고 한다.
이미 claims[msg.sender][_cToken] 값에 이전 값이 할당되어있기 때문에 Alice는 claim 단계에서부터 막힌다.

따라서 _redeem 함수 내부에 claims[msg.sender][_cToken]을 먼저 차감하는 식으로 하는 것이 낫지 않을까 싶다.

2. target의 토큰 밸런스를 확인하는 것은 효력이 없음

//MerkleRedeemerDripper.sol

contract MerkleRedeemerDripper is ERC20Dripper {
    constructor(
        address _core,
        address _target,
        uint256 _dripPeriod,
        uint256 _amountToDrip,
        address _token
    ) ERC20Dripper(_core, _target, _dripPeriod, _amountToDrip, _token) {}

    /// @notice Overrides drip() in the ERC20Dripper contract to add a balance check on the target
    /// @dev This will revert if there are < amountToDrip tokens in this contract, so make sure
    /// that it is funded with a multiple of amountToDrip to avoid this case.
    function drip() public override {
        require(
            IERC20(token).balanceOf(target) < amountToDrip,
            "MerkleRedeemerDripper: dripper target already has enough tokens."
        );

        super.drip();
    }
}

//ERC20Dripper.sol

    function drip() public virtual afterTime whenNotPaused {
        // reset timer
        _initTimed();

        // drip
        _withdrawERC20(token, target, amountToDrip);
        emit Dripped(amountToDrip);
    }

MerkleRedeemerDripper의 drip 함수는 단지 target의 토큰 밸런스가 보내고자 하는 양보다 작은지만 확인한다. 이 때 MerkleRedeemerDripper는 ERC20Dripper을 상속하는데, 기존 drip 함수를 살펴보면 출금을 요청하게 되어있다. 내가 생각하는 시나리오는 다음과 같다.

Bob은 공격 컨트랙트가 토큰을 받으면 fallback 함수를 이용해 다른 주소로 토큰을 보내고 다시 drip 함수를 호출하도록 컨트랙트를 만든다.
Bob은 target 주소를 자신이 만든 공격 컨트랙트 주소로 제출한다.
Alice는 MerkleRedeemerDripper 컨트랙트를 배포하고 drip 함수를 호출한다.
공격 컨트랙트에서 토큰을 받으면 fallback 함수가 작동하고 다른 주소로 토큰을 보내기 때문에 require문을 빠져나와 계속해서 MerkleRedeemerDripper 컨트랙트의 자금을 빼올 수 있다.

drip 함수의 require문을 수정하고 nonReentrant modifier를 추가하는 것이 좋을 것 같다.

3. 검산하는 코드가 추가되면 더 좋을 것 같음

//TribeRedeemer.sol

    function previewRedeem(uint256 amountIn)
        public
        view
        returns (address[] memory tokens, uint256[] memory amountsOut)
    {
        tokens = tokensReceivedOnRedeem();
        amountsOut = new uint256[](tokens.length);

        uint256 base = redeemBase;
        for (uint256 i = 0; i < tokensReceived.length; i++) {
            uint256 balance = IERC20(tokensReceived[i]).balanceOf(address(this));
            require(balance != 0, "ZERO_BALANCE");
            // @dev, this assumes all of `tokensReceived` and `redeemedToken`
            // have the same number of decimals
            uint256 redeemedAmount = (amountIn * balance) / base;
            amountsOut[i] = redeemedAmount;
        }
    }

    /// @notice Redeem `redeemedToken` for a pro-rata basket of `tokensReceived`
    function redeem(address to, uint256 amountIn) external nonReentrant {
        IERC20(redeemedToken).safeTransferFrom(msg.sender, address(this), amountIn);

        (address[] memory tokens, uint256[] memory amountsOut) = previewRedeem(amountIn);

        uint256 base = redeemBase;
        redeemBase = base - amountIn; // decrement the base for future redemptions
        for (uint256 i = 0; i < tokens.length; i++) {
            IERC20(tokens[i]).safeTransfer(to, amountsOut[i]);
        }

        emit Redeemed(msg.sender, to, amountIn, base);
    }

메이저까진 아니더라도 만일의 경우를 대비해서 추가하면 좋지 않을까 싶었던 코드다. redeem 함수를 호출하면 previewRedeem에서 redeemedAmount를 계산해서 나온 만큼 토큰을 전송한다. 이 때 아래 계산식을 사용한다. 저번 레포트에서 비슷한 결함을 봤던 것 같은데 항상 이더리움에서는 나눗셈을 조심해야 한다.

uint256 redeemedAmount = (amountIn * balance) / base;

분모가 분자보다 크면 나눗셈의 결과가 0이 되므로 결국 나중에 0개의 토큰을 전송할 수 있다. 따라서 해당 계산식 아래에 한 번 더 검산하는 require 문을 추가하는 것이 좋다고 생각한다. 조심해서 나쁠 건 없으니까.

require(redeemedAmount * base = amountIn * balance, "다시 계산이 필요합니당");

레포트

[M-02] TRIBEREDEEMER WILL START REDEEMING INCORRECTLY IF SOMEONE TRANSFER REDEEM TOKENS DIRECTLY TO IT

//TribeRedeemer.sol

    function previewRedeem(uint256 amountIn)
        public
        view
        returns (address[] memory tokens, uint256[] memory amountsOut)
    {
        tokens = tokensReceivedOnRedeem();
        amountsOut = new uint256[](tokens.length);

        uint256 base = redeemBase;
        for (uint256 i = 0; i < tokensReceived.length; i++) {
            uint256 balance = IERC20(tokensReceived[i]).balanceOf(address(this));
            require(balance != 0, "ZERO_BALANCE");
            // @dev, this assumes all of `tokensReceived` and `redeemedToken`
            // have the same number of decimals
            uint256 redeemedAmount = (amountIn * balance) / base;
            amountsOut[i] = redeemedAmount;
        }
    }

    /// @notice Redeem `redeemedToken` for a pro-rata basket of `tokensReceived`
    function redeem(address to, uint256 amountIn) external nonReentrant {
        IERC20(redeemedToken).safeTransferFrom(msg.sender, address(this), amountIn);

        (address[] memory tokens, uint256[] memory amountsOut) = previewRedeem(amountIn);

        uint256 base = redeemBase;
        redeemBase = base - amountIn; // decrement the base for future redemptions
        for (uint256 i = 0; i < tokens.length; i++) {
            IERC20(tokens[i]).safeTransfer(to, amountsOut[i]);
        }

        emit Redeemed(msg.sender, to, amountIn, base);
    }

내가 3번에서 생각했던 코드부분과 같지만 포인트가 조금 다르다. 사실 이 부분을 생각하긴 했었다. '누가 악의적으로 토큰을 넣으면 어떻게 되는거지?'라고 하면서 혼자 계산식에 숫자를 대입했었다. 그런데 아쉽게도 나는 컨트랙트에 있는 토큰 밸런스 대비 redeem되는 토큰의 갯수에 포커스를 맞췄었다. 이렇게 했을 때 컨트랙트 밸런스에 비례해서 output이 나오길래 별 문제가 없을 줄 알았다.

그런데 포커스를 맞췄어야 하는 부분은 input 대비 output 이었다. 지금 와서 다시 노트를 보니 input이 그대로인대 컨트랙트에 있는 토큰 밸런스에 비례해서 output이 증가한다. 즉 이 말은 누가 컨트랙트에 토큰을 직접적으로 넣어주면 넣기 전에 redeem한 사람보다 더 많은 토큰을 가져갈 수 있게된다. 이는 모든 토큰을 정해진 비율대로 분배하고자 하는 컨트랙트 취지에 벗어나게 된다.

생각의 시작은 좋았으나 엉뚱한 곳에 초점을 맞춘 것이 조금 아쉽다. 이번 레포트를 읽으면서 얻을 수 있는 교훈은 컨트랙트가 설계된 의도를 정확히 파악하는 것이 중요하다는 것. 그리고 의도를 벗어나서 동작하는 순간 이슈로 판단할 수 있다는 것. 마지막으로 내가 엉뚱한 곳에 초점을 맞추고 있는 것은 아닌지 한 번 더 생각해볼 것.