실제 Audit을 시행한 보고서를 읽는 것은 가장 많이 배울 수 있는 방법이다. 이번 글은 Secureum에서 작성한 글에 나온 사례들을 살펴보고 정리한 글이다.
3. Tokens with more than 18 decimal points will cause issues
function getSellRate(address _srcAddr, address _destAddr, uint _srcAmount, bytes memory) public override view returns (uint rate) {
(rate, ) = KyberNetworkProxyInterface(KYBER_INTERFACE)
.getExpectedRate(IERC20(_srcAddr), IERC20(_destAddr), _srcAmount);
// multiply with decimal difference in src token
rate = rate * (10**(18 - getDecimals(_srcAddr)));
// divide with decimal difference in dest token
rate = rate / (10**(18 - getDecimals(_destAddr)));
}가장 마지막 줄에 보면 토큰의 decimal이 18로 하드코딩되어있다. 대부분 erc20 토큰의 decimal은 18이지만 몇몇 토큰은 24인 것도 있다. 따라서 하드코딩하지 않고 해당 토큰의 decimal을 직접 가져오는 것이 좋다.
5. Reversed order of parameters in allowance function call
function pullTokens(
address _token,
address _from,
uint256 _amount
) internal returns (uint256) {
// handle max uint amount
if (_amount == type(uint256).max) {
uint256 allowance = IERC20(_token).allowance(address(this), _from);
uint256 balance = getBalance(_token, _from);
_amount = (balance > allowance) ? allowance : balance;
}
if (_from != address(0) && _from != address(this) && _token != ETH_ADDR && _amount != 0) {
IERC20(_token).safeTransferFrom(_from, address(this), _amount);
}
return _amount;
}allowance의 파라미터와 safeTransferFrom의 파라미터 순서가 다르다. -> data collision이 발생하는건가?
8. DAOfiV1Pair.deposit() accepts deposits of zero, blocking the pool
function deposit(address to) external override lock returns (uint256 amountBaseOut) {
require(msg.sender == router, 'DAOfiV1: FORBIDDEN_DEPOSIT');
require(deposited == false, 'DAOfiV1: DOUBLE_DEPOSIT');
reserveBase = IERC20(baseToken).balanceOf(address(this));
reserveQuote = IERC20(quoteToken).balanceOf(address(this));
// this function is locked and the contract can not reset reserves
deposited = true;
if (reserveQuote > 0) {
// set initial supply from reserveQuote
supply = amountBaseOut = getBaseOut(reserveQuote);
if (amountBaseOut > 0) {
_safeTransfer(baseToken, to, amountBaseOut);
reserveBase = reserveBase.sub(amountBaseOut);
}
}
emit Deposit(msg.sender, reserveBase, reserveQuote, amountBaseOut, to);
}위 코드는 단순히 baseToken과 quoteToken의 밸런스를 확인하고 deposited 값을 true로 바꿔버린다. 토큰 밸런스가 0인지 확인하지 않기 때문에 실제 deposit하지 않고도 pool이 잠기는 현상이 발생할 수 있다. 따라서 deposit하기 전에 nonzero인지 체크하고, 토큰의 최소양을 확인하는 코드가 추가되어야 한다.
9. GenesisGroup.commit overwrites previously-committed values
function commit(address from, address to, uint amount) external override onlyGenesisPeriod {
burnFrom(from, amount);
committedFGEN[to] = amount;
totalCommittedFGEN += amount;
emit Commit(from, to, amount);
}만약 다른 사람이 의도적으로 amount를 0으로 입력하면 이전에 commit 했던 committedFGEN[to] 값이 사라진다. 따라서 이전 값을 알 수 있도록 코드 변경이 필요하다.
17. ERC20 tokens with no return value will fail to transfer
if (!instance.transfer(getSendAddress(), forwarderBalance)) {
revert('Could not gather ERC20');
}ERC20 토큰을 transfer 하면 보통 boolean 값을 리턴한다. 따라서 위 코드 같은 경우는 정상적으로 작동해야 한다. 그런데 ERC20 토큰을 만들 때 의도적이든 의도적이지 않든 transfer 함수를 정의할 때 리턴 값을 정의하지 않는 경우도 있다. 각자 토큰을 어떻게 설계하는 지는 프로젝트마다 다를 수 있기 때문이다. 물론 IERC20을 준수하는 것이 바람직하다. 하지만 정상적으로 인터페이스를 준수하지 않는 토큰의 경우 위와 같이 해당 코드가 제대로 작동하지 않을 수 있다. 이를 방지하기 위해 SafeERC20을 사용하는 것을 권장하고 있다. 해당 표준은 토큰의 transfer가 boolean을 리턴하는지 확인하기 때문에 이를 방지할 수 있다. 자세한 사항은 아래 답변에 나와 있다.
21.Users can collect interest from SavingsContract by only staking mTokens momentarily
// 1. Only collect interest if it has been 30 mins
uint256 timeSinceLastCollection = now.sub(previousCollection);
if(timeSinceLastCollection > THIRTY_MINUTES) {mAsset을 예치하면 사용자는 credit 토큰을 받고 일정 이자율 대로 이자를 받을 수 있다. 여기까지는 문제가 없다. 하지만 위 코드는 이자율이 30분 동안 고정되도록 설정해놓았다. 이렇게 되면 악의적인 사용자는 30분이 끝나기 직전에 mAsset을 예치했다가 다음 시간에 이자율이 높아지면 바로 인출해서 이득을 취할 수 있다. 따라서 고정된 시간 값이 아닌 예치가 되는 순간마다 이자율이 변동되도록 수정해야 한다.
22. Oracle updates can be manipulated to perform atomic front-running attack
만약 갈라가 10/usd 의 비율로 현재 거래가 되고 있는데 체인링크 오라클에서 가격을 갑자기 15/usd로 바꾸는 트잭을 보낸다고 치자. 그러면 여기서 아비트라저는 총 2개의 트랜잭션을 보내서 이익을 취할 수 있다.
- 현재 가치의 갈라를 대량 매수하는 트랜잭션을 보낸다.(오라클 트잭보다 조금 더 높은 가스비를 지불하고 오라클 트잭 바로 앞으로 보냄)
- 갈라를 대량 매도하는 트랜잭션을 보낸다.(오라클 트잭보다 조금 더 낮은 가스비를 지불하고 오라클 트잭 바로 뒤로 보냄)
아비트라저는 10/usd 가격으로 대량 매수했다가 오라클이 바뀌자마자 15/usd 가격으로 대량 매도하는 방법으로 이익을 취할 수 있다. 이를 방지하기 위한 방법으로는 토큰의 가격을 유동성을 공급하거나 제거할 때 바로바로 받아오는 것이 아니라 블록이 한 번 생성될 때마다로 바꾸고 해당 가격을 블록 전체에 적용되게 하는 것이다. 조금의 지연된 가격으로 front-running 공격을 방지할 수 있다.
23. Certain functions lack input validation routines
function includeAsset (Shells.Shell storage shell, address _numeraire, address _numeraireAssim, address _reserve, address _reserveAssim, uint256 _weight) internal {
Assimilators.Assimilator storage _numeraireAssimilator = shell.assimilators[_numeraire];
_numeraireAssimilator.addr = _numeraireAssim;
_numeraireAssimilator.ix = uint8(shell.numeraires.length);
shell.numeraires.push(_numeraireAssimilator);
Assimilators.Assimilator storage _reserveAssimilator = shell.assimilators[_reserve];
_reserveAssimilator.addr = _reserveAssim;
_reserveAssimilator.ix = uint8(shell.reserves.length);
shell.reserves.push(_reserveAssimilator);
shell.weights.push(_weight.divu(1e18).add(uint256(1).divu(1e18)));
}checks-effects-interactions 패턴은 Reentrancy 공격을 방지할 때 사용하지만 다른 문제들도 방지할 수 있는 좋은 패턴이다. 위 코드는 인자로 들어오는 값들이 valid한 값인지 확인하는 단계가 없다. 악의적인 값이 아니더라도 비정상적인 값이 들어오면 함수가 의도한대로 작동하지 않을 수 있다. 따라서 interaction 전에 해당 값을 check 하는 단계가 필요하다.
25. A reverting fallback function will lock up all payouts
function _transferETH(address _recipient, uint256 _amount) private {
(bool success, ) = _recipient.call{value: _amount}(
abi.encodeWithSignature("")
);
require(success, "Transfer Failed");
}위 코드는 다수의 _recipient에게 송금하는 기능을 한다. 하지만 도중에 _recipient.call이 fail이 나면서 revert 된다면 나머지 사람들은 돈을 받을 수 없다. 이를 방지하기 위해 Pull over Push pattern을 사용하는 것이 좋다. 해당 글에 자세히 나와있어서 한 번 읽어보는 것을 추천한다.
요점은 송금할 주소를 확인하는 프로세스와 call이 발생하는 프로세스를 분리해서 withdraw 함수를 따로 만드는 것이다. 컨트랙트 내에서 한 번에 전송하는 패턴을 사용하지 않고 사용자들이 각자 withdraw 함수를 호출하게 함으로써 컨트랙트의 부담을 사용자들이 나눠 받는 형식을 사용해 리스크를 분산시킬 수 있다.
contract auction {
address highestBidder;
uint highestBid;
mapping(address => uint) refunds;
function bid() payable external {
require(msg.value >= highestBid);
if (highestBidder != address(0)) {
refunds[highestBidder] += highestBid; // record the refund that this user can claim
}
highestBidder = msg.sender;
highestBid = msg.value;
}
function withdrawRefund() external {
uint refund = refunds[msg.sender];
refunds[msg.sender] = 0;
(bool success, ) = msg.sender.call.value(refund)("");
require(success);
}
}30. Whitelisted tokens limit
for (uint256 i = 0; i < tokens.length; i++) {
uint256 amountToRagequit = fairShare(userTokenBalances[GUILD][tokens[i]], sharesAndLootToBurn, initialTotalSharesAndLoot);
// deliberately not using safemath here to keep overflows from preventing the function execution (which would break ragekicks)
// if a token overflows, it is because the supply was artificially inflated to oblivion, so we probably don't care about it anyways
userTokenBalances[GUILD][tokens[i]] -= amountToRagequit;
userTokenBalances[memberAddress][tokens[i]] += amountToRagequit;
}Whitelisted token이 많을 경우, 해당 기능에서 out of gas가 발생할 수 있다. 따라서 제한된 토큰만 화이트리스트에 추가하는 것을 권장한다. 추가된 코드는 다음과 같다.
uint256 constant MAX_TOKEN_WHITELIST_COUNT = 400; // maximum number of whitelisted tokens
uint256 constant MAX_TOKEN_GUILDBANK_COUNT = 200; // maximum number of tokens with non-zero balance in guildbank
uint256 public totalGuildBankTokens = 0; // total tokens with non-zero balance in guild bank37. Reentrancy and untrusted contract call in mintMultiple
function mintMultiple(
address[] calldata _assets,
uint256[] calldata _amounts
) external whenNotDepositPaused {
require(_assets.length == _amounts.length, "Parameter length mismatch");
uint256 priceAdjustedTotal = 0;
uint256[] memory assetPrices = _getAssetPrices(false);
for (uint256 i = 0; i < allAssets.length; i++) {
for (uint256 j = 0; j < _assets.length; j++) {
if (_assets[j] == allAssets[i]) {
if (_amounts[j] > 0) {
uint256 assetDecimals = Helpers.getDecimals(allAssets[i]);
uint256 price = assetPrices[i];
if (price > 1e18) {
price = 1e18;
}
priceAdjustedTotal += _amounts[j].mulTruncateScale(
price,
10**assetDecimals
);
}
}
}
}
// Rebase must happen before any transfers occur.
if (priceAdjustedTotal > rebaseThreshold && !rebasePaused) {
rebase(true);
}
for (uint256 i = 0; i < _assets.length; i++) {
IERC20 asset = IERC20(_assets[i]);
asset.safeTransferFrom(msg.sender, address(this), _amounts[i]);
}
oUSD.mint(msg.sender, priceAdjustedTotal);
emit Mint(msg.sender, priceAdjustedTotal);
if (priceAdjustedTotal >= autoAllocateThreshold) {
allocate();
}
}위 함수는 Reentrancy guard가 없고 제로 밸런스 체크가 없다. asset을 민팅하는 함수를 짤 때는 항상 Reentrancy guard를 사용하는 것이 좋다.mintMultiple 함수는 배열을 인자로 받기 때문에 공격자는 배열 안에 첫번째는 정삭적인 asset을 넣더라도 두 번째는 악의적인 asset을 넣을 수 있다. 악의적으로 만든 asset의 safeTransferFrom을 customize 해서 계속해서 토큰을 민팅할 수 있다. 이는 컨트랙트 내의 일시적인 imbalance를 만들어내고 이를 통해 rebase된 컨트랙트를 이용해 이익을 취할 수 있다.
44. Lack of a contract existence check allows token theft
위 코드는 토큰의 주소를 인자로 받아서 low-level call을 통해 전송한다. solidity docs의 warning 부분을 보면 account가 존재하지 않더라도 call , delegatecall , staticcall은 true를 리턴할 수 있다고 나와있다. 따라서 인자로 넣어준 토큰의 주소가 존재하지 않더라도 해당 함수는 실행될 수 있다. 이는 해커가 다른 사람이 등록할 토큰의 주소를 미리 알 경우에 문제가 발생한다.
시나리오는 다음과 같다. Bob이 열심히 컨트랙트를 짜서 이제 토큰을 배포하려고 한다. Eve는 배포될 토큰의 컨트랙트를 미리 알아내서 Hermez에 해당 주소를 등록하고 허위로 만든 쉿코인을 대량 입금한다. Bob이 토큰을 배포하고 Hermez에 등록 후 토큰을 입금하는 순간 Eve는 이전에 입금했던 쉿코인의 갯수만큼 Bob의 토큰을 인출할 수 있다.
그런데 배포될 토큰의 컨트랙트 주소를 미리 알 수 있는 방법이 정말 있을까? 실제로 컨트랙트 주소를 미리 아는 방법이 존재하며 생각보다 간단하다. 배포할 지갑 주소와 논스값만 알면 된다. 심지어 미리 알려주는 사이트도 개발됐으니 궁금하면 한 번 찾아보길 바란다.
따라서 이와 같은 문제를 방지하기 위해 인자로 받는 컨트랙트의 주소가 이미 배포된 주소인지 확인하는 절차가 필요하다.
47. Lack of two-step procedure for critical operations leaves them error-prone
권한을 위임하거나 새로운 지갑 주소를 등록할 때 주의가 필요하다. 특히 권한을 위임할 지갑 주소가 잘못 등록 된다면 이미 권한은 넘어간 뒤이기 때문에 돌이킬 수 없다. 따라서 주소를 등록하는 단계를 세분화해야 한다. 예를 들어 지갑 주소를 먼저 등록하고 해당 주소에서 권환을 승인하는 방식으로 진행하면 이러한 문제를 방지할 수 있을 것이다.
49. Missing validation of _owner argument could indefinitely lock owner role
위 코드처럼 constructor에서 owner 주소를 따로 설정할 수 있다. 하지만 주소가 잘못 입력되는 것을 방지하기 위해 msg.owner를 owner로 먼저 설정하고 이후 다른 주소로 권한을 위임하는 것이 바람직하다. 권환을 위임할 때는 앞 예시처럼 단계를 세분화해야 한다.
출처 및 참고자료
