오딧 시리즈 시작하며,
솔리디티 책도 2권 정도 읽었고, https://docs.soliditylang.org/ 문서도 궁금한 부분은 다 챙겨보았다.
그동안 스터디를 하면서 Treasure Dao 프로젝트의 Market, Mining 컨트랙트와 Magic Dragon Dao 프로젝트의 Staking & Reward 관련 컨트렉트를 분석해봤는데, 이제는 코드에서 특별히 신택스나 솔리디티 동작을 모르는 경우가 많지는 않았다.
그래서 좀 더 많은 프로젝트의 코드를, 특히 security와 관련된 부분을 보기 위해서 오픈제플린의 오딧 리포트를 보고 인사이트를 좀 남겨보기로 했다. 오딧 리포트의 모든 항목을 살펴보기 보단 공부 하면서 도움이 될 만한 것들만 볼 예정이다. 리포트에는 단순한 오타나 계산 실수같은것들도 많아서 전체를 다 보는건 큰 의미가 없을 것 같다.
EIP-4337 – Ethereum Account Abstraction Audit
이 시리즈의 첫번째 포스트는 EIP-4337 – Ethereum Account Abstraction Audit 이다. 이더리움에서 계정에 추가적인 기능이 들어가는것으로 보인다. 실제로 제안을 살펴본 것은 아니지만, 오딧 리포트를 보니 stake등의 기능이 생기는 것 같다.
Deposit manipulation
- audit
Firstly, the new funds are added to the caller’s current balance instead of the current account balance. This effectively allows anyone to delete the deposit from any account.
- fix
Fixed in pull request #50. The addStakeTo function was renamed to addStake and updated such that the caller can only add value to their own stake.
- note
addStakeTo 함수에서 address파라미터를 가지고 deposit mapping에 접근이 가능하다. 따라서 누구나 다른사람 계정의 deposit을 가지고 staking을 할 수 있다. address를 키로 가지는 mapping 변수에 대한 access가 있을 때 에는 msg.sender 혹은 onlyOwner 같은 modifier 로 privilege 체크가 잘 되어있는지 확인해야 한다.
Token transfers may fail silently
- audit
The DepositPaymaster
ignores the token transfer return value when adding deposits, withdrawing tokens from the contract and recovering gas costs. Although many tokens revert on failure, the token standard only specifies a boolean return value indicating success or failure. For tokens that return false, such as the 0x Protocol Token, these transfers may fail silently, leading to incorrect internal accounting.
- fix
Fixed in pull request #54. The DepositPaymaster contract now uses OpenZeppelin’s [SafeERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.5.0/contracts/token/ERC20/utils/SafeERC20.sol) library functions for token transfers.
- note:
ERC20 에서 transfer 함수의 return값이 true/false인지 체크해봐야 한다. 혹은 오픈제플린의 SafaERC20 을 쓴다.(실패 시 throw 함)
Use of transfer function is potentially unsafe
- audit
The withdrawTo function in StakeManager and the transfer function in SimpleWallet both uses Solidity’s built-in transfer function (on line 129 and line 52, respectively) to send ether to a destination address. The use of transfer for this purpose is no longer recommended.
Consider using the call function or OpenZeppelin’s sendValue function, and adhere to the checks-effects-interactions pattern when sending value to an external address. This pattern is already implemented in the compensate function of the EntryPoint contract.
Update: Partially fixed in pull request #57. The SimpleWallet contract’s transfer function was left unchanged.
- fix
Partially fixed in pull request #57. The SimpleWallet contract’s transfer function was left unchanged.
- note
LINK 에 따르면 solidity의 내장 함수인 transfer는 더이상 쓰는것이 권장되지 않는다. call을 쓰거나 OpenZeppelin의 라이브러리가 제공하는 function을 쓰는게 좋다.
Ref
https://blog.openzeppelin.com/eth-foundation-account-abstraction-audit/